-
Notifications
You must be signed in to change notification settings - Fork 228
improve telemetry api initialization and tests #2023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve telemetry api initialization and tests #2023
Conversation
|
Thank you for the work @herin049, I'm definitely more in favour of this approach, getting rid of the random port selection. Have you been running your version of the collector in a production environment? |
Thank you for the review @wpessers! Unfortunately, we have not been running these change in production, but we have ran them in our lower environments to validate that the changes fix the original bug. After a little bit of digging, it seems like other Lambda extension authors opt to use this approach as well, in particular the NewRelic extension: https://github.com/newrelic/newrelic-lambda-extension/blob/2a5c012f5082d2f8f056252e87d2d3eecad5479a/lambda/logserver/logserver.go#L270 Unfortunately, AWS does not seem to give any guidance on if it is preferable to bind to port |
|
@herin049 The listener code looks good to me, just some comments / questions I'd like answered before moving forward with this one. Again disclaimer that I'm far from an expert at go, also the reason why it took me some time to get back to you. |
|
I am OK with the changes here |
f031c9d to
cb4c892
Compare
|
@wpessers I've address all your original comments on the unit tests and made some additional improvements. |
wpessers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adressed concerns wherever relevant, looks good to me!
|
Thank you for contributing @herin049 🚀 |
A few months ago my team began noticing occasional
bind: address already in useerrors showing up on cold starts when using the Lambda collector layer (see #1740). We were able to come up with a fix internally, but since then it looks like the issue has been mostly solved by #1751. However, from a code maintainability/personal taste perspective the fix implemented is not ideal since it addresses the issue naively by running a brute force search over the ephemeral port range. A more optional solution is to just bind to port0which will allow the OS to automatically assign an available ephemeral port (see https://man7.org/linux/man-pages/man7/ip.7.html) This PR switches to using this approach and adds a basic set of tests for the telemetry API listener. Any additional comments are welcome as always.