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

[Sampler.AWS] Wrapped root sampler with ParentBasedSampler #2188

Merged
merged 14 commits into from
Nov 18, 2024

Conversation

AsakerMohd
Copy link
Contributor

Fixes #
Design discussion issue #

Changes

The current implementation of the X-Ray sampler has 2 levels: The X-Ray Sampler itself which is made up two different samplers for X-Ray purposes. Those two different samplers are wrapped by parentBasedSampler so that the children inherit the parent sampling decision. However, on the CloudWatch console, we are adding the children sampling decision into the stats section. This means: if we have 1 parent and 9 children, the CW console will show that 10 spans are sampled which isn't correct. It should show only 1 span being sampled. This PR moves the ParentBasedSampler wrapper to be on the root level of the X-Ray sampler. This means that the builder doesn't return AWSXrayRemoteSampler anymore but returns ParentBasedSampler(new AWSXrayRemoteSampler). This keeps the same parentBasedSampling logic but is instead on the root level thus skipping the stat aggregation for the children.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@AsakerMohd AsakerMohd requested a review from a team as a code owner October 7, 2024 23:33
@github-actions github-actions bot requested review from ppittle and srprash October 7, 2024 23:33
@github-actions github-actions bot added the comp:sampler.aws Things related to OpenTelemetry.Samplers.AWS label Oct 7, 2024
Copy link

@vastin vastin left a comment

Choose a reason for hiding this comment

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

LGTM

@AsakerMohd AsakerMohd requested a review from Kielek October 9, 2024 18:10
@Kielek Kielek changed the title Wrapped root sampler with ParentBasedSampler [Sampler.AWS] Wrapped root sampler with ParentBasedSampler Oct 10, 2024
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.25%. Comparing base (71655ce) to head (56e5f14).
Report is 606 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2188       +/-   ##
===========================================
+ Coverage   73.91%   88.25%   +14.34%     
===========================================
  Files         267       20      -247     
  Lines        9615      562     -9053     
===========================================
- Hits         7107      496     -6611     
+ Misses       2508       66     -2442     
Flag Coverage Δ
unittests-Sampler.AWS 88.25% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...lemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs 83.33% <100.00%> (+0.98%) ⬆️
src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs 66.66% <100.00%> (+11.11%) ⬆️
...c/OpenTelemetry.Sampler.AWS/SamplingRuleApplier.cs 99.26% <100.00%> (+0.70%) ⬆️

... and 261 files with indirect coverage changes

---- 🚨 Try these New Features:

@Kielek Kielek requested a review from ppittle October 25, 2024 07:05
Copy link
Contributor

github-actions bot commented Nov 2, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 2, 2024
@Kielek
Copy link
Contributor

Kielek commented Nov 4, 2024

@ppittle, do you want to merge this?

@AsakerMohd, I think that having CHANGELOG entry is good idea here.

@github-actions github-actions bot removed the Stale label Nov 5, 2024
@AsakerMohd
Copy link
Contributor Author

@ppittle, do you want to merge this?

@AsakerMohd, I think that having CHANGELOG entry is good idea here.

Yeah let me do that.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added Stale and removed Stale labels Nov 13, 2024
@ppittle
Copy link
Member

ppittle commented Nov 18, 2024

@AsakerMohd - LGTM - thanks for the changes!

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

Approving ane mergin based on @ppittle comment.

@Kielek Kielek merged commit 3e8b607 into open-telemetry:main Nov 18, 2024
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:sampler.aws Things related to OpenTelemetry.Samplers.AWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants