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

OpenTelemetryAutoConfiguration should use parent-based sampler by default #33821

Closed
belovaf opened this issue Jan 13, 2023 · 3 comments
Closed
Assignees
Labels
type: bug A general bug
Milestone

Comments

@belovaf
Copy link

belovaf commented Jan 13, 2023

OpenTelemetryAutoConfiguration uses a simple probability sampler.
With this strategy we can potentialy (if probability is not set to 100%) get inconsistent trace because some services decide not to sample their spans.
More over a brave sampler has parent-based behavior by default.

From open telemetry sampler documentation:

/**
 * Returns a new TraceIdRatioBased {@link Sampler}. The ratio of sampling a trace is equal to that
 * of the specified ratio.
 *
 * <p>The algorithm used by the {@link Sampler} is undefined, notably it may or may not use parts
 * of the trace ID when generating a sampling decision. Currently, only the ratio of traces that
 * are sampled can be relied on, not how the sampled traces are determined. As such, it is
 * recommended to only use this {@link Sampler} for root spans using {@link
 * Sampler#parentBased(Sampler)}.
 *
 * @param ratio The desired ratio of sampling. Must be within [0.0, 1.0].
 * @return a new TraceIdRatioBased {@link Sampler}.
 * @throws IllegalArgumentException if {@code ratio} is out of range
 */
static Sampler traceIdRatioBased(double ratio) {
  return TraceIdRatioBasedSampler.create(ratio);
}

I think it would be nice if you replace

Sampler.traceIdRatioBased(properties.getSampling().getProbability())

with

Sampler rootSampler = Sampler.traceIdRatioBased(properties.getSampling().getProbability());
return Sampler.parentBased(rootSampler);
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 13, 2023
@belovaf belovaf closed this as completed Jan 13, 2023
@belovaf belovaf closed this as completed Jan 13, 2023
@belovaf belovaf reopened this Jan 13, 2023
@philwebb
Copy link
Member

@shakuzen Do you have any advice for the suggested change?

@philwebb philwebb added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Jan 13, 2023
@wilkinsona wilkinsona changed the title OpenTelemetryAutoConfiguration should user parent-based sampler by default OpenTelemetryAutoConfiguration should use parent-based sampler by default Jan 13, 2023
@shakuzen
Copy link
Member

Yes, the proposed change is sensible. I suspect that's the behavior we intended and we merely missed that it was not the actual behavior. /cc @marcingrzejszczak @jonatan-ivanov

@marcingrzejszczak
Copy link
Contributor

Yes, we missed that, sorry

@mhalbritter mhalbritter added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team labels Jan 13, 2023
@mhalbritter mhalbritter modified the milestones: 3.0.2, 3.0.x Jan 13, 2023
@mhalbritter mhalbritter self-assigned this Jan 19, 2023
@mhalbritter mhalbritter modified the milestones: 3.0.x, 3.0.2 Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

6 participants