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

Implementing xrayTraceIDRatioBasedsampler #3090

Closed

Conversation

alanprot
Copy link

Signed-off-by: Alan Protasio approtas@amazon.com

Hi..

The default Otel TraceIDRatioBased does not work with XRay as it make the decision based on the first 8 bytes of the traceId -> which are not random on traces generated with the xray.NewIDGenerator().

This PR creates a similar sampler that works on XRAY as it makes the decision based on the last 8 bytes of the trace id.

@codecov
Copy link

codecov bot commented Dec 16, 2022

Codecov Report

Merging #3090 (09be06f) into main (2f95a2b) will increase coverage by 0.1%.
The diff coverage is 93.7%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3090     +/-   ##
=======================================
+ Coverage   69.0%   69.2%   +0.1%     
=======================================
  Files        147     148      +1     
  Lines       6884    6916     +32     
=======================================
+ Hits        4754    4787     +33     
  Misses      2011    2011             
+ Partials     119     118      -1     
Impacted Files Coverage Δ
samplers/aws/xray/traceid_ratio_sampler.go 93.7% <93.7%> (ø)
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 83.9% <0.0%> (+0.9%) ⬆️

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Just a couple nits.

samplers/aws/xray/go.mod Show resolved Hide resolved
samplers/aws/xray/traceid_ratio_sampler.go Outdated Show resolved Hide resolved
alanprot and others added 4 commits December 20, 2022 22:20
Signed-off-by: Alan Protasio <approtas@amazon.com>
Signed-off-by: Alan Protasio <approtas@amazon.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
@alanprot alanprot force-pushed the xrayTraceIDRatioBasedSampler branch from d2d150f to 8a88536 Compare December 21, 2022 06:25
@willarmiros
Copy link
Contributor

willarmiros commented Dec 23, 2022

Thanks for creating this PR! This is interesting to see, because I know in Java the TraceIdRatioBased sampler reads from the trailing bytes rather than the leading bytes by default. We even have a test for it. So it makes me wonder, is the correct fix to make a new sampler for X-Ray IDs only (which may be very confusing for customers to use), or could we change the default traceIdRatioBased sampler to just read from the trailing bytes in the main Go SDK?

@alanprot
Copy link
Author

Thanks for creating this PR! This is interesting to see, because I know in Java the TraceIdRatioBased sampler reads from the trailing bytes rather than the leading bytes by default. We even have a test for it. So it makes me wonder, is the correct fix to make a new sampler for X-Ray IDs only (which may be very confusing for customers to use), or could we change the default traceIdRatioBased sampler to just read from the trailing bytes in the main Go SDK?

I had the same question but I guess I took the safer route.. idk if changing the default one will break some other use cases ..

What the others think ?

@Aneurysm9
Copy link
Member

I think it would be ideal to fix in the base sampler. I'm not aware of anything that should break if the position of the sampled bits changes. open-telemetry/opentelemetry-go#3557 may replace this PR.

}
}

func (s *traceIDRatioBasedSampler) ShouldSample(p sdktrace.SamplingParameters) sdktrace.SamplingResult {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a note for exported func.

@tomwans
Copy link

tomwans commented Dec 27, 2022

Take a look at a related PR: open-telemetry/opentelemetry-go#3557 which brings the Go SDK in line with the Java and Python SDKs, which all by default sample from the trailing bytes.

@Aneurysm9
Copy link
Member

This is fixed via open-telemetry/opentelemetry-go#3557.

@Aneurysm9 Aneurysm9 closed this Dec 30, 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.

5 participants