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

WithNewRoot uses the parent context for sampling decisions. #2031

Closed
dashpole opened this issue Jun 25, 2021 · 2 comments · Fixed by #2032
Closed

WithNewRoot uses the parent context for sampling decisions. #2031

dashpole opened this issue Jun 25, 2021 · 2 comments · Fixed by #2032
Labels
area:trace Part of OpenTelemetry tracing bug Something isn't working

Comments

@dashpole
Copy link
Contributor

Description

If I specify WithNewRoot, the parent span context is not used when getting the trace ID, or generating the new span id. However, it is still used if you have a ParentBased sampler.

WithPublicEndpoint() would seem to imply that we don't trust incoming span contexts, and my expectation was that we wouldn't want to use them to make sampling decisions.

WithNewRoot(), on the other hand, would seem to only imply that the spans started are root spans. It doesn't necessarily imply anything about sampling (although the specification seems to below).

IMO, we should either:

  1. Change WithNewRoot() to store the newly created parent span context in the context before passing the context to samplers.
  2. Document that WithPublicEndpoint still uses incoming span contexts for making sampling decisions, and recommend against using ParentBased samplers with public endpoints.

The specification would seem to (implicitly) support 1, since it ties having no parent to being a root span.

@Aneurysm9
Copy link
Member

	var psc trace.SpanContext
	if o.NewRoot() {
		ctx = trace.ContextWithSpanContext(ctx, psc)
	} else {
		psc = trace.SpanContextFromContext(ctx)
	}

Is this what you're thinking for option 1? Seems right to me.

@dashpole
Copy link
Contributor Author

Yeah, thats what i'm suggesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants