Skip to content
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

Merged
merged 16 commits into from
Feb 10, 2022
Merged

Allow for tagging internal SDK metrics #76

merged 16 commits into from
Feb 10, 2022

Conversation

deosu
Copy link
Contributor

@deosu deosu commented Feb 4, 2022

Update newProxySender to take an additional setter with Otel tag set by
the Otel-Collector.

Update newProxySender to take an additional setter with Otel tag set by
the Otel-Collector.
@deosu deosu requested a review from oppegard February 4, 2022 18:27
senders/proxy.go Outdated Show resolved Hide resolved
@oppegard oppegard changed the title feat: Add a tag toreport usage of opentelemetry Allow for tagging internal SDK metrics Feb 4, 2022
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.
Copy link
Contributor

@keep94 keep94 left a 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.

senders/client_factory.go Outdated Show resolved Hide resolved
senders/client_test.go Show resolved Hide resolved
senders/proxy_test.go Outdated Show resolved Hide resolved
deosu and others added 6 commits February 7, 2022 10:37
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.
1. Add an option to provide custom reportTicker
2. Add an unit test
toverify SDKMetricsTags
internal/registry.go Outdated Show resolved Hide resolved
internal/registry.go Outdated Show resolved Hide resolved
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.
@deosu deosu requested a review from oppegard February 9, 2022 20:24
Remove an option to set custom value for reportTicker, as it is causing
the race condition.
Copy link
Contributor

@oppegard oppegard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@deosu deosu merged commit 4d4d9a7 into master Feb 10, 2022
@deosu deosu deleted the MONIT-26080 branch February 10, 2022 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants