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 clearing the span from scope #1025

Closed
marcingrzejszczak opened this issue Sep 28, 2020 · 7 comments
Closed

Allow clearing the span from scope #1025

marcingrzejszczak opened this issue Sep 28, 2020 · 7 comments
Labels
area:api Cross language API specification issue question Question for discussion release:after-ga Not required before GA release, and not going to work on before GA spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory

Comments

@marcingrzejszczak
Copy link

In Spring Cloud Sleuth we have the following situation with scheduled methods. We instrument ExecutorServices that schedule tasks to be executed in separate threads. At the point of instrumenting we have no idea what that task really is (e.g. it may end up to be a @Scheduled annotated method). Currently, with Brave, users have the option to define a property to pick which @Scheduled annotated methods should be ignored, thus not instrumented. So what we do is we clear the tracing context

@Around("execution (@org.springframework.scheduling.annotation.Scheduled  * *.*(..))")
	public Object traceBackgroundThread(final ProceedingJoinPoint pjp) throws Throwable {
		if (this.skipPattern != null && this.skipPattern
				.matcher(pjp.getTarget().getClass().getName()).matches()) {
			// we might have a span in context due to wrapping of runnables
			// we want to clear that context
			this.tracer.withSpanInScope(null);
			return pjp.proceed();
		}
		...
	}

When I look at the OTel API for Tracer I see that I can't clear the scope (null can't be passed).

/**
   * Enters the scope of code where the given {@link Span} is in the current Context, and returns an
   * object that represents that scope. The scope is exited when the returned object is closed.
   * .....
   *
   * @param span The {@link Span} to be set to the current Context.
   * @return an object that defines a scope where the given {@link Span} will be set to the current
   *     Context.
   * @throws NullPointerException if {@code span} is {@code null}.
   * @since 0.1.0
   */
  @MustBeClosed
  Scope withSpan(Span span);

I would propose clearing that requirement that NPE should be thrown when span is null. It's a valid case to perform a later decision within the lifecycle of the span about putting it out of the scope.

@marcingrzejszczak marcingrzejszczak added the spec:context Related to the specification/context directory label Sep 28, 2020
@Oberon00
Copy link
Member

Hi, this is not a spec issue (or if it is, it should be handled in #1020). In Java, the way to clear a span is to pass a non-null invalid span. That should work something like withSpan(DefaultSpan.create(SpanContext.getInvalid())).

@Oberon00 Oberon00 added area:api Cross language API specification issue question Question for discussion release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory labels Sep 28, 2020
@marcingrzejszczak
Copy link
Author

marcingrzejszczak commented Sep 28, 2020

Why isn't is a spec issue? I want to be able to clear the scope on the API level.

In Java, the way to clear a span is to pass a non-null invalid span. That should work something like withSpan(DefaultSpan.create(SpanContext.getInvalid())).

Maybe that's worth documenting in the Javadoc? Also it turns out that one can do:

tracer.withSpan(DefaultSpan.getInvalid());

Maybe instead of DefaultSpan.getInvalid() there should be sth like DefaultSpan.getClearScope() or sth like that?

@anuraaga
Copy link
Contributor

@marcingrzejszczak As far as I know, not all languages have the concept of a Scope so it's not considered a spec issue. Very happy to improve the documentation and/or add helpers for this, opened open-telemetry/opentelemetry-java#1717 to track for Java.

@Oberon00
Copy link
Member

Let's see, there are actually two bits required from the spec:

  1. There needs to be a way to set the span in a context. This is already being addressed by Clarify where Context.Key lives, and API to insert/extract from a Context instance #1020
  2. There needs to be a way to create an invalid span. The spec already has the concept of a "Propagated Span", and Remove restriction that SpanContext must be a final class, specify creation. #969 clarified that there must also be an API to create a SpanContext that can be feed to that propagated span (we don't specify the details, but the ability to create an invalid SpanContext is IMHO implicit).

So I'd say the spec has you covered.

And as @anuraaga mentioned Scope and withSpan are just convenience helper methods in Java, and not defined at all at spec level.

@marcingrzejszczak
Copy link
Author

Oh, I didn't know that not everyone has a concept of a scope. If that's the case let's continue this discussion here open-telemetry/opentelemetry-java#1717

@magicliang
Copy link

magicliang commented Feb 17, 2023

I have an issue like this, but I am not sure should I issue my question here, let me try to describe my problem:

I have a Play-based application, which uses ebean as ORM framework.

When a request comes into my play controller, the OpenTelemetry java agent creates a root context and span for my application, the traceId and spanId are injected into thread automatically, which is good for me.

But when the request is over, the injected traceId and spanId are still left behind in thread and inherited child thread.

Then the problem is coming: ebean has some daemon threads aiming to do polling all the time to keep the database connection is alive. daemon threads are injected traceId and keep printing legacy traceid. And the polling spans keep to be sended to remote collector. So simple request may get a long long long trace tree after some time.

What is the recommended practice to clear these injected, legacy contexts?

@mateuszrzeszutek
Copy link
Member

Hey @magicliang ,
Can you create an issue in the opentelemetry-java-instrumentation project? Please describe your setup in detail, and if possible, include a reproduction scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue question Question for discussion release:after-ga Not required before GA release, and not going to work on before GA spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

5 participants