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

Allow Sampler overriding when starting a span #401

Closed
clippit opened this issue Dec 25, 2019 · 4 comments
Closed

Allow Sampler overriding when starting a span #401

clippit opened this issue Dec 25, 2019 · 4 comments
Milestone

Comments

@clippit
Copy link
Contributor

clippit commented Dec 25, 2019

Currently only DefaultSampler in Provider will be used when starting a new span, which is hard to customize. For some test purpose, one may start a must-sampled span(eg. via a specific query parameter in http request) without touching the default sampler. How about setting an alternative sampler as a StartOption so that it can be used during the startSpanInternal period?

@jmacd
Copy link
Contributor

jmacd commented Dec 26, 2019

I'd ask whether the Sampler API could accommodate this kind of decision making, instead of making it a span option. The answer is no, which I think points to a defect in the Sampler API.

	ShouldSample(
		sc core.SpanContext,
		remote bool,
		traceID core.TraceID,
		spanID uint64,
		spanName string,
	) Decision

I'd suggest that this API, which is only good for relatively simple decision making, isn't good enough (e.g., it doesn't take the span's starting attributes, which are critical to good sampling decisions IMO).

In the context of #381 which follows OTEP 66, I'd suggest that we change the signature of Sampler to take a context.Context instead of the first four parameters above. That is to say that after #381 / OTEP 66 we have all of this information available in the context.

Then you'd be able to create a new Sampler implementation that looks for additional information in the context, such as a bit determined by a URL parameter.

@clippit
Copy link
Contributor Author

clippit commented Jan 2, 2020

@jmacd My original thought is using different Samplers for different spans rather than one Sampler for all circumstances. If staring attributes considered in decision making, it could also provide a way to determine sampling or not by a URL parameter somehow.

But open-telemetry/oteps#66 seems not related to Sampler API. In OTEP 0006 which defines Sampler API, it shows that initial set of Attributes for the Span being constructed should be considered to produce a sampling result. Maybe we can start with this approach?

@jmacd
Copy link
Contributor

jmacd commented Jan 2, 2020

You mean to add the starting attributes as a parameter to the sampling API? That works -- until someone points out the next thing it's missing. If you wanted to use the (distributed) correlation context for example, another signature change would be needed.

The relationship with open-telemetry/oteps#66 is that everything will be accessible from the Context, possibly we should use the Context in the sampling API. That can be a future step, but we'll end up changing the API a lot if we change it for every missing parameter.

@rghetia
Copy link
Contributor

rghetia commented Mar 26, 2020

please open a spec issue.

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

No branches or pull requests

4 participants