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

Remote sampling support - AWS X-Ray #1859

Merged
merged 53 commits into from Apr 1, 2022

Conversation

bhautikpip
Copy link
Contributor

This PR introduces remote sampling support using AWS X-Ray. This implementation uses 2 X-Ray APIs as mentioned below.

  1. getSamplingRules (retrieve customer set rules on AWS X-Ray console)
  2. getSamplingTargets (retrieve targets assigned to each client for each rule but API request should also send a snapshot on how many requests are sample, borrowed and matched using each rule)

Reference docs for some of the data structures used in implementation
https://docs.aws.amazon.com/xray/latest/devguide/xray-console-sampling.html#xray-console-config
https://docs.aws.amazon.com/xray/latest/api/API_GetSamplingRules.html
https://docs.aws.amazon.com/xray/latest/api/API_GetSamplingTargets.html

@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #1859 (1fc8df8) into main (5611141) will increase coverage by 0.8%.
The diff coverage is 75.3%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1859     +/-   ##
=======================================
+ Coverage   68.5%   69.4%   +0.8%     
=======================================
  Files        124     135     +11     
  Lines       5366    6113    +747     
=======================================
+ Hits        3680    4243    +563     
- Misses      1582    1742    +160     
- Partials     104     128     +24     
Impacted Files Coverage Δ
samplers/aws/xray/timer.go 0.0% <0.0%> (ø)
samplers/aws/xray/remote_sampler.go 2.1% <2.1%> (ø)
samplers/aws/xray/rand.go 23.0% <23.0%> (ø)
samplers/aws/xray/internal/client.go 79.6% <79.6%> (ø)
samplers/aws/xray/internal/rule.go 82.2% <82.2%> (ø)
samplers/aws/xray/internal/manifest.go 90.2% <90.2%> (ø)
samplers/aws/xray/remote_sampler_config.go 91.4% <91.4%> (ø)
samplers/aws/xray/internal/match.go 92.3% <92.3%> (ø)
samplers/aws/xray/fallback_sampler.go 97.8% <97.8%> (ø)
samplers/aws/xray/internal/clock.go 100.0% <100.0%> (ø)
... and 1 more

@bhautikpip
Copy link
Contributor Author

@anuraaga @MrAlias @jmacd @Aneurysm9 Can you review this PR? I have closed the older one and opened a fresh PR with complete remote sampling support and tried to address most of the concerns/comments we have discussed. Feel free to take a look again. Thanks!

samplers/aws/xray/fallback_sampler_test.go Show resolved Hide resolved
samplers/aws/xray/internal/client.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/client.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/manifest.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/manifest.go Show resolved Hide resolved
samplers/aws/xray/internal/manifest.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/manifest.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/reservoir.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/rule.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/util/rand.go Outdated Show resolved Hide resolved
Copy link
Member

@hanyuancheung hanyuancheung left a comment

Choose a reason for hiding this comment

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

PTAL at the suggestion.

samplers/aws/xray/internal/manifest.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/match.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/match_test.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/reservoir.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/rule.go Outdated Show resolved Hide resolved
samplers/aws/xray/remote_sampler_config.go Outdated Show resolved Hide resolved
samplers/aws/xray/remote_sampler_config.go Outdated Show resolved Hide resolved
samplers/aws/xray/remote_sampler_test.go Outdated Show resolved Hide resolved
samplers/aws/xray/remote_sampler_test.go Outdated Show resolved Hide resolved
samplers/aws/xray/internal/manifest.go Outdated Show resolved Hide resolved
@bhautikpip bhautikpip requested a review from MrAlias March 28, 2022 16:52
@bhautikpip
Copy link
Contributor Author

bhautikpip commented Mar 28, 2022

While running the make precommit I have started receiving this error. There is some checksum mis match in deps of tools module. Are we planning to fix this issue as part of the other PR ?

bhautip@38f9d362ae67 opentelemetry-go-contrib % make precommit     
cd ./tools && \
        go build -o /Users/bhautip/Documents/lambda/opentelemetry-go-contrib/.tools/dbotconf go.opentelemetry.io/build-tools/dbotconf
verifying github.com/blizzy78/varnamelen@v0.6.1/go.mod: checksum mismatch
        downloaded: h1:mGBHm+Uo4e8JnZEKHRoZgVEOQdSBdQfY/x+k4NAXBWA=
        go.sum:     h1:zy2Eic4qWqjrxa60jG34cfL0VXcSwzUrIx68eJPb4Q8=

@Aneurysm9
Copy link
Member

That version should be retracted. Not sure why it is being used. You may be able to work around it temporarily by putting an exclude directive for it in tools/go.mod.

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MrAlias
Copy link
Contributor

MrAlias commented Mar 28, 2022

One final unresolved issue as far as I see: #1859 (comment)

Otherwise this is looking good 👍

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@bhautikpip bhautikpip requested a review from MrAlias March 29, 2022 17:48
@bhautikpip
Copy link
Contributor Author

bhautikpip commented Mar 29, 2022

Also, not sure why linter check is failing. I'm able to run it in my local. Maybe re-running it would be helpful since it's some timeout issue.

@bhautikpip
Copy link
Contributor Author

bhautikpip commented Mar 29, 2022

@MrAlias can you run tests again? looks like some flaky test failure. If you don't have further comments we would be okay to merge this?

@MrAlias
Copy link
Contributor

MrAlias commented Mar 29, 2022

@MadVikingGod PTAL

@bhautikpip
Copy link
Contributor Author

bhautikpip commented Apr 1, 2022

@MadVikingGod Can you take a look again to see if you have further comments on this PR or this is good to merge?

@Aneurysm9
Copy link
Member

I spoke with @MadVikingGod and he's good with this proceeding. He will ask for follow-up PRs if he has any concerns that need to be addressed.

@Aneurysm9 Aneurysm9 merged commit 8cf7954 into open-telemetry:main Apr 1, 2022
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.

None yet

6 participants