Skip to content

Respecting -tracing.otel.sample-ratio configuration when enabling OpenTelemetry tracing with X-ray #4862

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

Merged
merged 4 commits into from
Sep 15, 2022

Conversation

alanprot
Copy link
Member

@alanprot alanprot commented Sep 14, 2022

What this PR does:
Creates a simple sampler as the TraceIDRatioBased does not work well with xray generated Ids.

See: https://pkg.go.dev/go.opentelemetry.io/contrib/propagators/aws/xray#section-readme

It is a general suggestion to not use the traceIDRatioSampler while also using the X-Ray IDGenerator. The non-random nature of building an X-Ray traceId may lead to unexpected sampling results.

This sampler simple randomly pick traces to be sampled based on the fraction received as parameter. The sample decision is then propagated via the parentBased sampler.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • [NA] Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Alan Protasio <approtas@amazon.com>
@alanprot alanprot force-pushed the tracing-simple-sampler branch from a7dd01c to 85b75fc Compare September 14, 2022 19:30
Signed-off-by: Alan Protasio <approtas@amazon.com>
@alanprot alanprot changed the title [OTEL Tracing] Respecting -tracing.otel.sample-ratio configuration when enabling OpenTelemetry tracing with X-ray Sep 14, 2022
@alanprot alanprot marked this pull request as ready for review September 14, 2022 19:36
Signed-off-by: Alan Protasio <approtas@amazon.com>
Signed-off-by: Alan Protasio <approtas@amazon.com>
@alanprot alanprot force-pushed the tracing-simple-sampler branch from 8debfc5 to 10a3ea1 Compare September 15, 2022 06:04
@alanprot alanprot merged commit 6c2d013 into cortexproject:master Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants