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

Ensure all XRay Sampler functionality is under ParentBased logic #6205

Closed
wants to merge 3 commits into from

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Oct 4, 2024

This fixes the bug where:

  • Currently, XRay Sampler in OTel Go will use ShouldSample logic for every span, but for child spans, they should always respect the parent span sampling decision in XRay Sampler (as is already done in OTel Java and OTel .NET)

The changes introduced in this PR are as follows:

  • Wrap the returned remote_sampler of NewRemoteSampler under a single ParentBased Sampler

Existing Issue(s):
Related

Testing:

@jj22ee jj22ee requested a review from a team as a code owner October 4, 2024 23:23
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.7%. Comparing base (64a1336) to head (80f4bb0).
Report is 10 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6205     +/-   ##
=======================================
+ Coverage   67.4%   67.7%   +0.2%     
=======================================
  Files        206     206             
  Lines      13220   13220             
=======================================
+ Hits        8922    8958     +36     
+ Misses      3996    3957     -39     
- Partials     302     305      +3     
Files with missing lines Coverage Δ
samplers/aws/xray/remote_sampler.go 37.3% <100.0%> (+34.3%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

👎
Ensuring this is the responsibility of the developer using this sampler in their own application.
This change prevents folks from setting up their own parent-based logic and using that.

@MrAlias
Copy link
Contributor

MrAlias commented Oct 7, 2024

Thank you for your interest in the project. We have recently removed this sampler (#6187) as it does not have an owner. I'm closing this as we are no longer accepting changes to this code.

@MrAlias MrAlias closed this Oct 7, 2024
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.

4 participants