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

Add request object influenced sampling option #182

Merged
merged 2 commits into from Oct 8, 2018

Conversation

Projects
None yet
6 participants
@rakyll
Copy link
Member

rakyll commented Sep 22, 2018

Request object influenced sampling also allows
us to implement blacklist/whitelist features.


There are two ways to control the `Sampler` used:
* Controlling the global default `Sampler` via [TraceConfig](https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/TraceConfig.md).
* Pass a specific `Sampler` as an option to the HTTP plugin. Plugins should support setting

This comment has been minimized.

@SergeyKanzhelev

SergeyKanzhelev Sep 24, 2018

Contributor

Can you please elaborate on scenarios when per-request sampler is needed?

This comment has been minimized.

@SergeyKanzhelev

SergeyKanzhelev Sep 24, 2018

Contributor

One scenario may be a sampler that is registered by exporter to filter out calls to that exporter endpoint. Is it intended use for this API?

This comment has been minimized.

@SergeyKanzhelev

SergeyKanzhelev Sep 25, 2018

Contributor

BTW, can this callback be used to extract additional information from http object and set a new attribute on span?

This comment has been minimized.

@derekperkins

derekperkins Sep 25, 2018

The simplest scenario is to always sample when ?debug=true or however that is propagated through a system. Other scenarios might be to always turn on tracing for developer requests or specific clients.

This comment has been minimized.

@rakyll

rakyll Sep 25, 2018

Author Member

I added some examples, PTAL.

The only use case is not just disabling tracing but being able to give user availability to use different policies per path. E.g. some endpoints with low traffic may get a higher sampling rate.

This is why I think it is better to influence the sampling policy here rather than building exporter filters. We also care about the sampling option to be set because it will be propagated.

@derekperkins, thanks for the example. I added it to the list as well.

This comment has been minimized.

@bogdandrutu

bogdandrutu Sep 25, 2018

Member

@SergeyKanzhelev I proposed this #186 as a way to pass attributes from the Sampler to the Span.

```
type Transport struct {
// GetStartOptions allows to set start options per request.
GetStartOptions func(*http.Request) trace.StartOptions

This comment has been minimized.

@SergeyKanzhelev

SergeyKanzhelev Sep 24, 2018

Contributor

I can see many uses for a more generic filter/sampler that works on collected span. Those would be easier to write and they will work independently from what auto-collector was enabled.

This comment has been minimized.

@rakyll

rakyll Sep 25, 2018

Author Member

Do you mean a hook to create a custom span in the HTTP integration? Or a filter/sampler once the span is collected?

This comment has been minimized.

@SergeyKanzhelev

SergeyKanzhelev Sep 25, 2018

Contributor

I mean second. Let's take Zipkin exporter. When you configure it - you need to remember now to configure all http collectors with specialized sampler (side note- are those chained? I might need to apply different policies per path AND filter out Zipkin). So Zipkin exporter needs to have separate packages with the reference on http libraries (think Java) that would return those samplers.

It would be much easier if Zipkin can only implement one filter that works on collected spans.

This comment has been minimized.

@SergeyKanzhelev

SergeyKanzhelev Sep 25, 2018

Contributor

Good alternative here if Zipkin before calling into endpoint will set a spacial tag on TLS that will have special treatment by collection modules.

This comment has been minimized.

@rakyll

rakyll Sep 26, 2018

Author Member

Can you give few examples why a different sampling policy might be required per exporter? I haven't considered that case in this spec change.

This comment has been minimized.

@rakyll

rakyll Sep 26, 2018

Author Member

One more thing. Each exporter is depending on a different protocol. Some do HTTP, some do gRPC, some Thrift over UDP, etc. I think there is no way to unify this given they are all based on different protocols. Each exporter should provide a way to set a policy.

This comment has been minimized.

@rakyll

rakyll Sep 27, 2018

Author Member

@SergeyKanzhelev, do you have any additional comments on this?

I filed #188 to discuss more how filtering capabilities will work for exporters. I think they can be implemented once but set at multiple places because exporters' transports vary and request objects are transport specific.

This comment has been minimized.

@SergeyKanzhelev

SergeyKanzhelev Sep 28, 2018

Contributor

Yes. If we take filtering out of conversation - many scenarios may be easier to implement for customer if filters would not be protocol-specific and will only deal with populated Span object. Than you can register global callback that will apply different sampling for different http.path attribute independently of how it was constructed - using manual instrumentation or via automatic collection.

This note though doesn't make proposed callback less interesting. This callback makes it harder for customer to configure, but achieve better perf.

I'm good with this proposal

This comment has been minimized.

@SergeyKanzhelev

SergeyKanzhelev Sep 28, 2018

Contributor

Do we need a separate item for global processors?

This comment has been minimized.

@adriancole

adriancole Oct 5, 2018

TLDR; would be nice to focus this just on instrumentation sampling and not on things like exporter rate limiting etc.

side note, so take into consideration.. you don't have to answer each thought below :P

I don't really understand the link in this conversation about zipkin, exporter and instrumentation policy.

In Zipkin we don't recommend doing any sampling collector side except consistent on trace ID for overload scenarios. Note that pruning leaf spans (like resources) is a special case and not really a sampling decision, also there's no collector hook for that (even if there could be). A better way to address things like resource spans is to do that in the instrumentation layer. For example, sleuth has an exclude pattern for things like this that uses our http sampler to achieve that.

Almost always when we talk about sampling in zipkin we are talking about instrumentation. We don't combine rates (via pack-ins) either at the moment though you could imagine something that does this. I don't think the first implementation of sampling in census needs to deal with combinatory effects of rate, rather api design.

Maybe later works can address the combinatory things and how throttling could be effective. Throttling is complex when local intermediate spans (or tons of log-like events) are used as they can saturate transport/ storage budget as well. I think census has some fields that would need to be used like dropped spans etc. basically pruning etc probably is a different pull request/feature than top-level trace tier sampling.

@rakyll rakyll force-pushed the rakyll:http-sampling branch from 030eb06 to 7425601 Sep 25, 2018

@rakyll
Copy link
Member Author

rakyll left a comment

PTAL.


There are two ways to control the `Sampler` used:
* Controlling the global default `Sampler` via [TraceConfig](https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/TraceConfig.md).
* Pass a specific `Sampler` as an option to the HTTP plugin. Plugins should support setting

This comment has been minimized.

@rakyll

rakyll Sep 25, 2018

Author Member

I added some examples, PTAL.

The only use case is not just disabling tracing but being able to give user availability to use different policies per path. E.g. some endpoints with low traffic may get a higher sampling rate.

This is why I think it is better to influence the sampling policy here rather than building exporter filters. We also care about the sampling option to be set because it will be propagated.

@derekperkins, thanks for the example. I added it to the list as well.

```
type Transport struct {
// GetStartOptions allows to set start options per request.
GetStartOptions func(*http.Request) trace.StartOptions

This comment has been minimized.

@rakyll

rakyll Sep 25, 2018

Author Member

Do you mean a hook to create a custom span in the HTTP integration? Or a filter/sampler once the span is collected?

@SergeyKanzhelev

This comment has been minimized.

Copy link
Contributor

SergeyKanzhelev commented Sep 28, 2018

@rakyll btw, this is how it works in Application Insights SDK today: http://apmtips.com/blog/2018/09/26/filtering-bad-sampling-good/ There are many perf tradeoff, but it definitely very easy for customer to configure and understand

@rakyll

This comment has been minimized.

Copy link
Member Author

rakyll commented Oct 8, 2018

@bogdandrutu can you review please?

Show resolved Hide resolved trace/HTTP.md Outdated
Show resolved Hide resolved trace/HTTP.md Outdated

rakyll added some commits Sep 22, 2018

Add request object influenced sampling option
Request object influenced sampling also allows
us to implement blacklist/whitelist features.

@rakyll rakyll force-pushed the rakyll:http-sampling branch from 97ebbc6 to 7ae3810 Oct 8, 2018

@rakyll rakyll merged commit 4173983 into census-instrumentation:master Oct 8, 2018

1 check passed

cla/google All necessary CLAs are signed

@rakyll rakyll deleted the rakyll:http-sampling branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.