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

Fix handling of baggage when retrieving current span #8567

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Mar 26, 2024

Description

Resolves #8548

OpenTelemetry separates baggage from spans and stores them separately in the OTel context. In contrast, the Helidon tracing API treats baggage as a characteristic of a span.

Earlier PRs fixed a problem in which Helidon failed to store the baggage associated with a span correctly in the OTel context. This PR fixes a problem in reconstituting the correct current span from what is in the OTel context, making sure the new reconstituted span draws from both the OTel span in the context and the OTel baggage in the context.

The central fix is in OpenTelemetryTracerProvider and its currentSpan method (which now delegates to activeSpan so the Jaeger code can also use it). The Span constructed for currentSpan/activeSpan now holds a reference to the OTel Baggage instance from the Otel Context.

Normally, that baggage will be our writeable baggage implementation of OTel's Baggage interface. That implementation allows the Helidon Span wrapper around the OTel span to permit updates to its baggage. By storing a reference to that baggage in the reconstituted span, changes to the baggage via the baggage methods on the reconstituted Span will write through to the baggage that is stored in the context.

But, it's possible that immutable Otel baggage might be in the OTel context when the current span is reconstructed. The Helidon OpenTelemetrySpan method for updating baggage therefore must now check to see if its baggage is writeable or not and act accordingly.

The PR also contains new tests for OTel and Jaeger (which relies on the OTel implementation internally).

Documentation

Bug fix; no doc changes.

Signed-off-by: Tim Quinn <tim.quinn@oracle.com>
…ence so changes to the span are reflected in the context's baggage
@tjquinno tjquinno self-assigned this Mar 26, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 26, 2024
@@ -100,7 +107,12 @@ public Scope activate() {
public Span baggage(String key, String value) {
Objects.requireNonNull(key, "baggage key cannot be null");
Objects.requireNonNull(value, "baggage value cannot be null");
baggage.baggage(key, value);
if (baggage instanceof MutableOpenTelemetryBaggage mutableBaggage) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there maybe a way to switch to mutable baggage here and make it work, or is this a limitation of the open telemetry API?
If we have this check + exception, then an application that works fine with other tracing provider may fail with oTel

@tjquinno tjquinno merged commit 37a8530 into helidon-io:helidon-3.x Mar 27, 2024
12 checks passed
@tjquinno tjquinno deleted the 3.x-jaeger-baggage-prop branch March 27, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants