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

Option to disable tracing centrally #1552

Closed
jakob-o opened this issue Aug 18, 2020 · 21 comments
Closed

Option to disable tracing centrally #1552

jakob-o opened this issue Aug 18, 2020 · 21 comments
Labels
blocked:spec blocked on open or unresolved spec Feature Request Suggest an idea for this project release:after-ga

Comments

@jakob-o
Copy link

jakob-o commented Aug 18, 2020

Is your feature request related to a problem? Please describe.
I would like to be able to disable tracing centrally as proposed in this API issue open-telemetry/opentelemetry-specification#530

Original motivation:
As already mentioned here open-telemetry/opentelemetry-specification#173 I'd like to be able to exclude or sample a list of URLs / URL-Patterns from instrumentation. In my case particularly to avoid generating many events from health- and liveness-checks.

Describe the solution you'd like
If I understood correctly in the python implementation the Context flag suppress_instrumentation indicates that the current context should not be instrumented.
What could be a proper implementation in the Java API / SDK?

Would it be suitable to implement it as a flag in TraceFlags and make SpanBuilderSdk#startSpan return a DefaultSpan as it would if the sampling decision was negative?
This flag could maybe set in HttpServerTracer depending on some configurable input?

Describe alternatives you've considered
Unfortunately I am not very familiar with the OTEL architecture and any hint to where certain information and or decisions should be made or where I am able to get a high level view of the Java SDK is highly appreciated.

@jakob-o jakob-o added the Feature Request Suggest an idea for this project label Aug 18, 2020
@jkwatson
Copy link
Contributor

Hi @jakob-o . Thanks for this request.

If you're using the auto-instrumentation agent, then this issue would probably belong in that repo: https://github.com/open-telemetry/opentelemetry-java-instrumentation

Otherwise, the instrumentation you apply is totally up to you, unless I'm misunderstanding the request.

@jakob-o
Copy link
Author

jakob-o commented Aug 18, 2020

Hey @jkwatson,

maybe you are right and this belongs to the instrumentation repository, but if I understood correctly in the API discussion it might be beneficial to have a general option such as a context flag to disable tracing centrally which can be set anywhere for example in a filter which would lead to generating noop-spans. Otherwise every (auto) instrumentation implementation would require to manually check somewhere (?) whether it should take place for the current request.

Does this make sense?

@jakob-o
Copy link
Author

jakob-o commented Aug 18, 2020

Sorry, the discussion related to the referenced issue i was referring to was actually taking place here open-telemetry/opentelemetry-specification#530

@iNikem
Copy link
Contributor

iNikem commented Aug 18, 2020

@jkwatson I think there was an idea to allow for Tracer to check for some flag/configuration and return non-recording/invalid span from startSpan

@jkwatson
Copy link
Contributor

Hmm. I'm trying to figure out if the instrumentation needs a feature to do this. If the instrumentation is making the decision, based on instrumentation configuration, can't the instrumentation choose to not create the span at all in the first place?

@jakob-o
Copy link
Author

jakob-o commented Aug 18, 2020

I think the issue is, that instrumentation might take place at several layers and if the HTTP instrumentation does decide to not create a span the JDBC instrumentation will not know this, unless it implements the same logic and will create a new span itself.
This would not be necessary if the tracer itself has means to disable it centrally.

@iNikem
Copy link
Contributor

iNikem commented Aug 18, 2020

The problem is not local to one instrumentation making a decision. E.g. my servlet instrumentation can detect excluded url pattern and create non-recording span. But after that there is not point in creating proper child spans for this SERVER span, is it? All instrumentations now must check if parent span is non-recording and probably skip creating any spans at all.

@jkwatson
Copy link
Contributor

The problem is not local to one instrumentation making a decision. E.g. my servlet instrumentation can detect excluded url pattern and create non-recording span. But after that there is not point in creating proper child spans for this SERVER span, is it? All instrumentations now must check if parent span is non-recording and probably skip creating any spans at all.

This feels like a separate issue (not unimportant!). Don't we have this issue today with all kinds of parents? Should the SDK not create child spans at all if the parent is non-recording?

Maybe I'm getting hung up on the name of this Issue. This seems like a much broader issue than excluding URLs from tracing in HTTP server spans. :)

@jakob-o
Copy link
Author

jakob-o commented Aug 18, 2020

The decision making should surely be implemented in the instrumentation. My question is: How do i correctly disable recording for the span / trace?
Does this feature require to make every instrumentation check the span, or would a central way to disable it as discussed here open-telemetry/opentelemetry-specification#530 make more sense?

@jakob-o
Copy link
Author

jakob-o commented Aug 18, 2020

@jkwatson would you like me to rename / rephrase the issue to something like Option to disable tracing centrally?

@trask
Copy link
Member

trask commented Aug 18, 2020

It sounds like this is proposing to implement open-telemetry/opentelemetry-specification#530? I'm all for this, but would be great if we can get this into the spec.

@jkwatson
Copy link
Contributor

That spec issue is marked as after-ga, so I'm going to follow suit for this one, for the nonce. If it ends up getting fast-tracked, we can change it here, as well.

@jakob-o jakob-o changed the title Exclude URLs from Tracing Option to disable tracing centrally Aug 18, 2020
@Oberon00
Copy link
Member

Should the SDK not create child spans at all if the parent is non-recording?

I think it already implements that 😃 It's called ParentOrElse sampler.

@jakob-o
Copy link
Author

jakob-o commented Aug 18, 2020

@jkwatson Would you recommend opening another issue regarding the URL exclusion in https://github.com/open-telemetry/opentelemetry-java-instrumentation and reference this issue?

@Oberon00 Would it be sufficient then to create a non-recording span in the HTTP instrumentation and create / enable this sampler? If so, do you know how and where would I do this appropriately?

The sampler is by default used as a decorator, if i understand correctly.

@jkwatson
Copy link
Contributor

yes, I think having an issue open for the instrumentation itself would be great.

There are configuration options in the agent to set the sampling setup; Configuration options are here: https://github.com/open-telemetry/opentelemetry-java-instrumentation#trace-config

@anuraaga
Copy link
Contributor

This feels like a separate issue (not unimportant!). Don't we have this issue today with all kinds of parents? Should the SDK not create child spans at all if the parent is non-recording?

Just to clarify this point, currently we create IDs for all spans, even non-recording ones. I think this is to allow the IDs to be injected into logs, for example. If the spans weren't valid, they wouldn't be injected nor would children be created.

@iNikem
Copy link
Contributor

iNikem commented Aug 19, 2020

I don't think we should create spans nor spanIds for "ignored" requests. I don't know if the correct way to that is via non-recording spans or with TraceContext.sampled=false

@jaisonsteephen
Copy link

@jakob-o , by exporting OTEL_JAVAAGENT_ENABLED=false, you can disable trace in java

@kmendo
Copy link

kmendo commented Jun 21, 2021

@jakob-o , by exporting OTEL_JAVAAGENT_ENABLED=false, you can disable trace in java

Where do you get this from? there is no reference for this flag in the code, nor it is documented.

@jkwatson
Copy link
Contributor

@jakob-o , by exporting OTEL_JAVAAGENT_ENABLED=false, you can disable trace in java

Where do you get this from? there is no reference for this flag in the code, nor it is documented.

this is in the instrumentation codebase, not in this one.

@jkwatson
Copy link
Contributor

I'm going to close this issue, as it is an instrumentation concern, not an SDK concern. If we get an spec change approved to support this, please re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:spec blocked on open or unresolved spec Feature Request Suggest an idea for this project release:after-ga
Projects
None yet
Development

No branches or pull requests

8 participants