Baggage is covered

[crossoverJie]

Describe the bug

I have set the baggage in the previous span, but I can not get it in current span.

Steps to reproduce

public class DemoSpanProcessor implements SpanProcessor {


    @Override
    public void onStart(Context parentContext, ReadWriteSpan span) {
        System.out.println("===========BeginSpan===========");
		System.out.println("SpanProcessor=====value: " + Baggage.current().asMap());

        Baggage.current().toBuilder()
                    .put(UPSTREAM_NAME, span.getName())
                    .build()
                    .storeInContext(Context.current()).makeCurrent();
        System.out.println("SpanProcessor=====set baggage: " + span.getName());


    }
}

java  \
-javaagent:opentelemetry-javaagent.jar \
-Dotel.javaagent.extensions=context-1.0-SNAPSHOT.jar \
-Dotel.traces.exporter=otlp \
-Dotel.propagators=tracecontext,baggage,demo \
-Dotel.exporter.otlp.endpoint=http://127.0.0.1:5317 \
      -jar target/demo-0.0.1-SNAPSHOT.jar --server.port=8181 --grpc.server.port=8282

Expected behavior

we can get the name of the previous span in the current span.

Actual behavior

The Baggage's data is empty.

Javaagent or library instrumentation version

1.32.0

Environment

JDK: openjdk version "21.0.2" 2024-01-16
OS: macOS

Additional context

After my debugging, I found that it was successfully set to threadlocal and then overwritten by other contexts.

There is a repo can reduce this issue.

[laurit]

After my debugging, I found that it was successfully set to threadlocal and then overwritten by other contexts.

You can't really modify the context in a span processor because the typical flow is

• get current context
• start span, use context from previous step as parent
• • run span processors
• add span to the context from first step
• make context from previous step current

even if you update the current context in a span processor it will get overwritten. You should do it in some other way. For example you could have a custom package propagator that adds the UPSTREAM_NAME field to baggage before delegating to W3CBaggagePropagator.
Also your current code does not close the scope returned from makeCurrent, this must be done.

[crossoverJie]

Thank you for your explanation, I understand.

My requirement is to pass the span information whose kind is equal to server in serverA to ServerB, so that serverB can know the source of its request, like this:

---

	// SpanProcessor
    public static final ThreadLocal<String> THREAD_PROPAGATION_UPSTREAM_NAME = new ThreadLocal<>();

    @Override
    public void onStart(Context parentContext, ReadWriteSpan span) {

        Thread t = Thread.currentThread();
        String upstreamName = parentContext.get(PROPAGATION_UPSTREAM_NAME);
        if (!StringUtils.isNullOrEmpty(upstreamName)) {
            span.setAttribute(UPSTREAM_NAME, upstreamName);
        }
        if (span.getKind() == SpanKind.SERVER) {
            String spanName = span.getName() + " " + span.getSpanContext().getSpanId();
            THREAD_PROPAGATION_UPSTREAM_NAME.set(spanName);
        }
    }

	// TextMapPropagator
    @Override
    public <C> void inject(Context context, C carrier, TextMapSetter<C> setter) {
        String upstreamName = THREAD_PROPAGATION_UPSTREAM_NAME.get();
        THREAD_PROPAGATION_UPSTREAM_NAME.remove();
        setter.set(carrier, FIELD, String.valueOf(upstreamName));
    }

    @Override
    public <C> Context extract(Context context, C carrier, TextMapGetter<C> getter) {
        String upstreamName = getter.get(carrier, FIELD);
        if (upstreamName != null) {
            return context.with(PROPAGATION_UPSTREAM_NAME, upstreamName);
        } else {
            return context;
        }
    }

Currently, I implement it through threadlocal. Is there any problem with this? Or is there a better implementation? Thank you so much.

[laurit]

Is there any problem with this?

@crossoverJie the obvious issue with your code are that context is propagated only once. For example if you make two http requests to external service than only the first would be handled. Also the code assumes that everything runs on the same thread.

[Cirilla-zmh]

@crossoverJie

According to the TextMapPropagator implementation that you showcased later on, it seems that you have not placed the upstreamName value into the OTel Tracing's Baggage. Instead, you've implemented a custom, cross-process context propagation mechanism.

If in your traces, the upstreamName always captures the value from the immediate upstream service, then such an implementation is reasonable. However, if the upstreamName always denotes the same value across the entire trace, you might want to consider storing it directly in the Baggage. (For reference, please see Baggage)

Furthermore, as @laurit pointed out, your storage of upstreamName internally leverages threadLocal, which presumes that the thread does not switch across the local call chain. In reality, using Context.with(key, value) might be a better choice, as it means you don't have to worry about thread switching.

[github-actions]

This has been automatically marked as stale because it has been marked as needing author feedback and has not had any activity for 7 days. It will be closed automatically if there is no response from the author within 7 additional days from this comment.