[processor/resourcedetection] ec2 detector start returns an error if network call fails #37426
Conversation
github-actions
bot
added
the
processor/resourcedetection
|
I am not very happy with this fix but this is the simplest change. It allows to make sure the collector only starts if it is initialized properly rather than reporting invalid information. It supposes that the collector will be restarted by the operating system after this failure. |
…detector fails to start because network is unreachable
atoulme
force-pushed
the
detect_network_error
branch
from
ec28237 to
452dad4
Compare
| var requestSendError *smithyhttp.RequestSendError | ||
| if errors.As(err, &requestSendError) { | ||
| d.logger.Warn("EC2 metadata endpoint unreachable", zap.Error(err)) | ||
| return pcommon.NewResource(), "", err |
There was a problem hiding this comment.
Can you please add a comment here saying why we return an error for this particular use case?
Also, are we sure that smithyhttp.RequestSendError is fired only when ec2 is in not ready state? Will it be a different error if it's not an ec2 instance?
Also, smithyhttp.RequestSendError might be unreliable. If the SDK is changed to use something else, we might miss it.
I'm thinking if we can have a configuration option instead. Happy to here other opinions.
There was a problem hiding this comment.
This looks good to me as the initial solution, if the first 2 Qs above are addressed
There was a problem hiding this comment.
I think I can work on adding an integration test to make sure this works as intended.
There was a problem hiding this comment.
Can you please add a comment here saying why we return an error for this particular use case?
I put a log warning above, can I maybe make that warning better, maybe more actionable? I will do that first. Comments tend to rot...
There was a problem hiding this comment.
I put a log warning above, can I maybe make that warning better, maybe more actionable? I will do that first. Comments tend to rot...
The warning is for the end user. When someone reads the code, it's unclear why this particular error should be propagated while others are not.
|
On further investigation, I think this approach is not valid and I will explain myself on #35936. Closing. |
Description
The resourcedetection processor fails to start the processor if the ec2 detector fails to start because network is unreachable
Link to tracking issue
Fixes #35936
Testing
Added a unit test.