-
Notifications
You must be signed in to change notification settings - Fork 21
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
Allow for tagging internal SDK metrics #76
Conversation
Update newProxySender to take an additional setter with Otel tag set by the Otel-Collector.
Update proxy.go & client.go to tag internal SDK metrics.
Add a delay in proxy_test.go to make sure connections are made before tests run.
Make the proxy variable local to tests in proxy_test.go.
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.
Thanks for doing this work Sumit. I went ahead and approved the changes since I will be out next week. That way I don't block you. However, I did have a few suggested changes for you, so please have a look at my comments. Have a great week.
1. Make a defensive copy of tags, so that the Option SDKMetricsTags() won't change the exisitng tags if the multiple maps for the SDKMetricsTags are provided. 2. Add a TODO to see if there is a better way to avoid the race condition.
Remove sleep from proxy_test.go as we are not able to reproduce the race-condition locally.
Add a channel to avoid the racing condition.
Make proxy_test more reliable
1. Add an option to provide custom reportTicker 2. Add an unit test toverify SDKMetricsTags
Reset the map to avoid race condition.
Reset the map to avoid race condition.
Reset the map to avoid race condition.
Reset the map to avoid race condition.
In client_test.go add assertions for the sent requests.
Remove an option to set custom value for reportTicker, as it is causing the race condition.
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.
Looks good!
Update newProxySender to take an additional setter with Otel tag set by
the Otel-Collector.