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

The default sampler should not sample all operations #30

Closed
carterkozak opened this issue Oct 31, 2018 · 7 comments
Closed

The default sampler should not sample all operations #30

carterkozak opened this issue Oct 31, 2018 · 7 comments

Comments

@carterkozak
Copy link
Contributor

What happened?

If I create a simple application and include http-remoting, every requests is sent with X-B3-Sampled: 1 due to our usage of AlwaysSampler by default. Even if my demonstration application does not write trace data to disk, it will cause other services to do so.

What did you want to happen?

I am proposing that we update the default sampler to new RandomSampler(.01f) to sample 1% of actions unless another sampler is specified. I have no strong opinion regarding 1% in particular, but would prefer that we do not sample everything. This value is used elsewhere, so it should be a reasonable default.

carterkozak added a commit to carterkozak/tracing-java that referenced this issue Oct 31, 2018
Previously the default configuration sampled every operation.
Any application using a tracing-aware client would send every
request with `X-B3-Sampled: 1`, so even if the local application
did not record sample data to disk, upstream services would
be forced to.
@markelliot
Copy link
Contributor

The goal of AlwaysSampler was to force deliberate configuration -- a roughly equivalent, but easier for your scenario, semantic would be a NeverSampler, which I think would be preferable to a preconfigured RandomSampler

@carterkozak
Copy link
Contributor Author

That's reasonable, will build shortly.

In the default case I'd prefer not to inform the target system whether or not to sample, but defer to its sampler. Implementation likely isn't worth the complexity though.

carterkozak added a commit to carterkozak/tracing-java that referenced this issue Oct 31, 2018
Previously the default configuration sampled every operation.
Any application using a tracing-aware client would send every
request with `X-B3-Sampled: 1`, so even if the local application
did not record sample data to disk, upstream services would
be forced to.
@uschi2000
Copy link

Yeah, I think I may have screwed that up in the Trace API: it doesn't account for "observability hasn't been decided" and thus the cannot choose to not set the SAMPLED header and defer the decision to the next node.

@carterkozak
Copy link
Contributor Author

There are a number of java api changes I'd like to propose for a major version revision. Where is the best place to track them, given that I don't have cycles to implement in the near future?

Examples:

  • Observability is undecided
  • Tracing API and Observer API should be isolated. Instrumenting a system with tracing should not require knowledge of Trace object.
  • Creating a new trace should always open a new span
  • Opening a span returns a token used to safely close that span, it should not be possible to close the wrong span.
  • span completion should not return the completed span (fastCompleteSpan semantics). When observation is disabled, there's no reason to profile execution.

@uschi2000
Copy link

I think it's fine to write up issues and tag them as major revision or 3.0.0 or similar. note that this will be particularly nasty to implement and roll out because of the interplay of static state (should keep same classes and packages in order avoid duplication) and API changes (should move to new packages like com.palantir.tracing3)...

@carterkozak
Copy link
Contributor Author

Great, thanks. Any chance I can get permissions to create GH milestones or labels?

@carterkozak
Copy link
Contributor Author

Closing in favor of #32

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

3 participants