From d222e569ae039d1983caba28dfd7ee66b9897ff5 Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Tue, 26 Mar 2024 17:43:24 -0500 Subject: [PATCH 1/2] Fix current span baggage handling Signed-off-by: Tim Quinn --- tracing/jaeger/pom.xml | 21 ++- .../tracing/jaeger/JaegerTracerProvider.java | 7 +- .../jaeger/JaegerBaggagePropagationTest.java | 136 ++++++++++++++++++ .../src/test/resources/application.yaml | 5 +- .../OpenTelemetryTracerProvider.java | 20 ++- .../opentelemetry/TestSpanAndBaggage.java | 85 +++++++++++ 6 files changed, 265 insertions(+), 9 deletions(-) create mode 100644 tracing/jaeger/src/test/java/io/helidon/tracing/jaeger/JaegerBaggagePropagationTest.java diff --git a/tracing/jaeger/pom.xml b/tracing/jaeger/pom.xml index 7e5f340888d..c4848058322 100644 --- a/tracing/jaeger/pom.xml +++ b/tracing/jaeger/pom.xml @@ -154,6 +154,11 @@ bedrock-testing-support test + + io.helidon.common.testing + helidon-common-testing-junit5 + test + @@ -166,13 +171,13 @@ default-test - **/B3HeadersPropagationCaseTest.java + **/B3HeadersPropagationCaseTest.java,**/JaegerBaggagePropagationTest.java - headers-propagation-test + b3-headers-propagation-test test @@ -182,6 +187,18 @@ + + + all-headers-propagation-test + + test + + + + **/JaegerBaggagePropagationTest.java + + + diff --git a/tracing/jaeger/src/main/java/io/helidon/tracing/jaeger/JaegerTracerProvider.java b/tracing/jaeger/src/main/java/io/helidon/tracing/jaeger/JaegerTracerProvider.java index 1398389a520..9b7cdb8eb7d 100644 --- a/tracing/jaeger/src/main/java/io/helidon/tracing/jaeger/JaegerTracerProvider.java +++ b/tracing/jaeger/src/main/java/io/helidon/tracing/jaeger/JaegerTracerProvider.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2022 Oracle and/or its affiliates. + * Copyright (c) 2019, 2024 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,11 +20,9 @@ import io.helidon.common.Prioritized; import io.helidon.tracing.Span; import io.helidon.tracing.Tracer; -import io.helidon.tracing.opentelemetry.HelidonOpenTelemetry; import io.helidon.tracing.opentelemetry.OpenTelemetryTracerProvider; import io.helidon.tracing.spi.TracerProvider; -import io.opentelemetry.context.Context; import jakarta.annotation.Priority; /** @@ -44,8 +42,7 @@ public void global(Tracer tracer) { @Override public Optional currentSpan() { - return Optional.ofNullable(io.opentelemetry.api.trace.Span.fromContextOrNull(Context.current())) - .map(HelidonOpenTelemetry::create); + return OpenTelemetryTracerProvider.activeSpan(); } @Override diff --git a/tracing/jaeger/src/test/java/io/helidon/tracing/jaeger/JaegerBaggagePropagationTest.java b/tracing/jaeger/src/test/java/io/helidon/tracing/jaeger/JaegerBaggagePropagationTest.java new file mode 100644 index 00000000000..442c7a808aa --- /dev/null +++ b/tracing/jaeger/src/test/java/io/helidon/tracing/jaeger/JaegerBaggagePropagationTest.java @@ -0,0 +1,136 @@ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.helidon.tracing.jaeger; + +import java.util.Optional; +import java.util.TreeMap; +import java.util.function.Supplier; + +import io.helidon.common.testing.junit5.OptionalMatcher; +import io.helidon.config.Config; +import io.helidon.tracing.HeaderConsumer; +import io.helidon.tracing.HeaderProvider; +import io.helidon.tracing.Scope; +import io.helidon.tracing.Span; +import io.helidon.tracing.SpanContext; +import io.helidon.tracing.Tracer; +import io.helidon.tracing.TracerBuilder; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; + +class JaegerBaggagePropagationTest { + + static final String BAGGAGE_KEY = "myKey"; + static final String BAGGAGE_VALUE = "myValue"; + + private static final String OTEL_AUTO_CONFIGURE_PROP = "otel.java.global-autoconfigure.enabled"; + private static final String OTEL_SDK_DISABLED_PROP = "otel.sdk.disabled"; + private static String originalOtelSdkAutoConfiguredSetting; + private static String originalOtelSdkDisabledSetting; + + private static Config config; + private static Tracer tracer; + + @BeforeAll + static void init() { + config = Config.create().get("tracing"); + tracer = TracerBuilder.create(config.get("jaeger-all-propagators")).build(); + originalOtelSdkAutoConfiguredSetting = System.setProperty(OTEL_AUTO_CONFIGURE_PROP, "true"); + originalOtelSdkDisabledSetting = System.setProperty(OTEL_SDK_DISABLED_PROP, "false"); + } + + @AfterAll + static void wrapup() { + if (originalOtelSdkAutoConfiguredSetting != null) { + System.setProperty(OTEL_AUTO_CONFIGURE_PROP, originalOtelSdkAutoConfiguredSetting); + } + if (originalOtelSdkDisabledSetting != null) { + System.setProperty(OTEL_SDK_DISABLED_PROP, originalOtelSdkDisabledSetting); + } + } + + @Test + void testBaggageProp() { + + var span = tracer.spanBuilder("testSpan").start(); + span.baggage(BAGGAGE_KEY, BAGGAGE_VALUE); + + checkBaggage(tracer, span, span::context); + } + + @Test + void testBaggageBeforeActivatingSpan() { + var span = tracer.spanBuilder("activatedTestSpan").start(); + span.baggage(BAGGAGE_KEY, BAGGAGE_VALUE); + + try (Scope scope = span.activate()) { + checkBaggage(tracer, span, this::currentSpanContext); + } + } + + @Test + void testBaggageAfterActivatingSpan() { + var span = tracer.spanBuilder("activatedTestSpan").start(); + + try (Scope scope = span.activate()) { + span.baggage(BAGGAGE_KEY, BAGGAGE_VALUE); + checkBaggage(tracer, span, this::currentSpanContext); + } + } + + private void checkBaggage(Tracer tracer, Span span, Supplier spanContextSupplier) { + try { + assertThat("Value after initial storage of baggage", + span.baggage(BAGGAGE_KEY), + OptionalMatcher.optionalValue(is(equalTo(BAGGAGE_VALUE)))); + + var headerConsumer = HeaderConsumer.create(new TreeMap<>(String.CASE_INSENSITIVE_ORDER)); + tracer.inject(spanContextSupplier.get(), HeaderProvider.empty(), headerConsumer); + + // Make sure the baggage header was set. + Optional baggageHeader = headerConsumer.get("baggage"); + assertThat("Baggage contents in propagated headers", + baggageHeader, + OptionalMatcher.optionalValue(is(equalTo(BAGGAGE_KEY + "=" + BAGGAGE_VALUE)))); + + // Now make sure the baggage is propagated to a new span context based on the header. + Optional propagatedSpanContext = tracer.extract(headerConsumer); + assertThat("Propagated span context", + propagatedSpanContext, + OptionalMatcher.optionalPresent()); + Span spanFromContext = tracer.spanBuilder("fromContext").parent(propagatedSpanContext.get()).build(); + assertThat("Baggage value from propagated span context", + spanFromContext.baggage(BAGGAGE_KEY), + OptionalMatcher.optionalValue(is(BAGGAGE_VALUE))); + + span.end(); + } catch (Exception ex) { + span.end(ex); + } + } + + private SpanContext currentSpanContext() { + return Span.current() + .map(Span::context) + .orElseThrow(() -> new IllegalStateException("No current span")); + } +} \ No newline at end of file diff --git a/tracing/jaeger/src/test/resources/application.yaml b/tracing/jaeger/src/test/resources/application.yaml index 297d850ecda..8bd3d62ea5d 100644 --- a/tracing/jaeger/src/test/resources/application.yaml +++ b/tracing/jaeger/src/test/resources/application.yaml @@ -1,5 +1,5 @@ # -# Copyright (c) 2019, 2023 Oracle and/or its affiliates. +# Copyright (c) 2019, 2024 Oracle and/or its affiliates. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -46,3 +46,6 @@ tracing: int-tags: tag5: 145 # JAEGER_TAGS tag6: 741 # JAEGER_TAGS + jaeger-all-propagators: + service: "prop-test" + propagation: ["jaeger", "b3", "w3c"] diff --git a/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java b/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java index 669826b854f..0ea9792a709 100644 --- a/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java +++ b/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java @@ -84,6 +84,24 @@ public static Tracer globalTracer() { return GLOBAL_TRACER.get(); } + /** + * Returns a {@link io.helidon.tracing.Span} representing the currently-active OpenTelemetry span with any current baggage + * set on the returned span. + * + * @return optional of the current span + */ + public static Optional activeSpan() { + // OTel returns a no-op span if there is no current one. + io.opentelemetry.api.trace.Span otelSpan = io.opentelemetry.api.trace.Span.current(); + Span helidonSpan = HelidonOpenTelemetry.create(otelSpan); + + // OTel returns empty baggage if there is no current one. + io.opentelemetry.api.baggage.Baggage otelBaggage = io.opentelemetry.api.baggage.Baggage.current(); + + otelBaggage.forEach((key, baggageEntry) -> helidonSpan.baggage(key, baggageEntry.getValue())); + return Optional.of(helidonSpan); + } + @Override public TracerBuilder createBuilder() { return OpenTelemetryTracer.builder(); @@ -105,7 +123,7 @@ public void global(Tracer tracer) { @Override public Optional currentSpan() { - return Optional.ofNullable(io.opentelemetry.api.trace.Span.current()).map(HelidonOpenTelemetry::create); + return activeSpan(); } @Override diff --git a/tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/TestSpanAndBaggage.java b/tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/TestSpanAndBaggage.java index 718a4281928..1060fef456a 100644 --- a/tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/TestSpanAndBaggage.java +++ b/tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/TestSpanAndBaggage.java @@ -18,20 +18,25 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.TreeMap; +import java.util.function.Supplier; import io.helidon.common.testing.junit5.OptionalMatcher; +import io.helidon.tracing.HeaderConsumer; import io.helidon.tracing.HeaderProvider; import io.helidon.tracing.Scope; import io.helidon.tracing.Span; import io.helidon.tracing.SpanContext; import io.helidon.tracing.Tracer; +import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; @@ -42,6 +47,10 @@ class TestSpanAndBaggage { private static String originalOtelSdkAutoConfiguredSetting; private static String originalOtelSdkDisabledSetting; + private static final String BAGGAGE_KEY = "mykey"; + private static final String BAGGAGE_VALUE = "myvalue"; + + @BeforeAll static void init() { originalOtelSdkAutoConfiguredSetting = System.setProperty(OTEL_AUTO_CONFIGURE_PROP, "true"); @@ -126,4 +135,80 @@ void testIncomingBaggage() { assertThat("Inbound baggage bag1", span.baggage("bag1"), OptionalMatcher.optionalValue(is("val1"))); assertThat("Inbound baggage bag1", span.baggage("bag2"), OptionalMatcher.optionalValue(is("val2"))); } + + + @Test + void testBaggageWithoutActivation() { + final var tracer = io.helidon.tracing.Tracer.global(); + final var span = tracer.spanBuilder("baggageCanaryMinimal").start(); + try { + // Set baggage and confirm that it's known in the span + span.baggage(BAGGAGE_KEY, BAGGAGE_VALUE); + checkBaggage(tracer, span, span::context); + } catch (final Exception x) { + span.end(x); + } finally { + span.end(); + } + } + + @Test + void testBaggageAddedBeforeActivation() { + final var tracer = io.helidon.tracing.Tracer.global(); + final var span = tracer.spanBuilder("baggageCanaryMinimal").start(); + + try { + // Set baggage and confirm that it's known in the span + span.baggage(BAGGAGE_KEY, BAGGAGE_VALUE); + try (Scope scope = span.activate()) { + checkBaggage(tracer, span, this::currentSpanContext); + } + } catch (final Exception x) { + span.end(x); + } finally { + span.end(); + } + } + + @Test + void testBaggageAddedAfterActivation() { + final String BAGGAGE_KEY = "mykey"; + final String BAGGAGE_VALUE = "myvalue"; + final var tracer = io.helidon.tracing.Tracer.global(); + final var span = tracer.spanBuilder("baggageCanaryMinimal").start(); + try { + // Set baggage and confirm that it's known in the span + try (Scope scope = span.activate()) { + span.baggage(BAGGAGE_KEY, BAGGAGE_VALUE); + checkBaggage(tracer, span, this::currentSpanContext); + } + } catch (final Exception x) { + span.end(x); + } finally { + span.end(); + } + } + + private void checkBaggage(Tracer tracer, Span span, Supplier spanContextSupplier) { + String value = span.baggage(BAGGAGE_KEY).orElseThrow(); + assertThat("baggage value right after set", value, Matchers.is(Matchers.equalTo(BAGGAGE_VALUE))); + + // Inject the span (context) into the consumer + final var consumer = HeaderConsumer + .create(new TreeMap<>(String.CASE_INSENSITIVE_ORDER)); + tracer.inject(spanContextSupplier.get(), HeaderProvider.empty(), consumer); + // Confirm that baggage was NOT propagated (the bug) + final var allKeys = consumer.keys(); + assertThat("Injected headers", allKeys, hasItem("baggage")); + assertThat("Injected baggage expression", + consumer.get("baggage"), + OptionalMatcher.optionalValue(Matchers.equalTo(BAGGAGE_KEY + "=" + BAGGAGE_VALUE))); + } + + private SpanContext currentSpanContext() { + return Span.current() + .map(Span::context) + .orElseThrow(() -> new IllegalStateException("No current span")); + } + } From 54f5ab8eb252960a90695a8d04ac434ce135f1d1 Mon Sep 17 00:00:00 2001 From: Tim Quinn Date: Tue, 26 Mar 2024 18:04:35 -0500 Subject: [PATCH 2/2] Don't copy baggage from context into active span; just save the reference so changes to the span are reflected in the context's baggage --- .../opentelemetry/HelidonOpenTelemetry.java | 14 +++++++++++++- .../tracing/opentelemetry/OpenTelemetrySpan.java | 16 ++++++++++++++-- .../OpenTelemetryTracerProvider.java | 8 ++++---- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/HelidonOpenTelemetry.java b/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/HelidonOpenTelemetry.java index e37f315f54e..d1979b0ae47 100644 --- a/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/HelidonOpenTelemetry.java +++ b/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/HelidonOpenTelemetry.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022, 2023 Oracle and/or its affiliates. + * Copyright (c) 2022, 2024 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,6 +22,7 @@ import io.helidon.config.Config; import io.opentelemetry.api.OpenTelemetry; +import io.opentelemetry.api.baggage.Baggage; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.Tracer; @@ -57,6 +58,17 @@ public static io.helidon.tracing.Span create(Span span) { return new OpenTelemetrySpan(span); } + /** + * Wrap an open telemetry span. + * + * @param span open telemetry span + * @param baggage open telemetry baggage + * @return Helidon (@link io.helidon.tracing.Span} + */ + public static io.helidon.tracing.Span create(Span span, Baggage baggage) { + return new OpenTelemetrySpan(span, baggage); + } + /** * Check if OpenTelemetry is present by indirect properties. diff --git a/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetrySpan.java b/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetrySpan.java index 2c04af201e5..2d73a62f9fd 100644 --- a/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetrySpan.java +++ b/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetrySpan.java @@ -23,6 +23,7 @@ import io.helidon.tracing.Span; import io.helidon.tracing.SpanContext; +import io.opentelemetry.api.baggage.Baggage; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.trace.StatusCode; @@ -30,10 +31,16 @@ class OpenTelemetrySpan implements Span { private final io.opentelemetry.api.trace.Span delegate; - private final MutableOpenTelemetryBaggage baggage = new MutableOpenTelemetryBaggage(); + private final Baggage baggage; OpenTelemetrySpan(io.opentelemetry.api.trace.Span span) { this.delegate = span; + baggage = new MutableOpenTelemetryBaggage(); + } + + OpenTelemetrySpan(io.opentelemetry.api.trace.Span span, Baggage baggage) { + delegate = span; + this.baggage = baggage; } @Override @@ -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) { + mutableBaggage.baggage(key, value); + } else { + throw new UnsupportedOperationException( + "Attempt to set baggage on a span with read-only baggage (perhaps from context"); + } return this; } diff --git a/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java b/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java index 0ea9792a709..6bb0d67a12b 100644 --- a/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java +++ b/tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022 Oracle and/or its affiliates. + * Copyright (c) 2022, 2024 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -93,13 +93,13 @@ public static Tracer globalTracer() { public static Optional activeSpan() { // OTel returns a no-op span if there is no current one. io.opentelemetry.api.trace.Span otelSpan = io.opentelemetry.api.trace.Span.current(); - Span helidonSpan = HelidonOpenTelemetry.create(otelSpan); // OTel returns empty baggage if there is no current one. io.opentelemetry.api.baggage.Baggage otelBaggage = io.opentelemetry.api.baggage.Baggage.current(); - otelBaggage.forEach((key, baggageEntry) -> helidonSpan.baggage(key, baggageEntry.getValue())); - return Optional.of(helidonSpan); + // Create the span directly with the retrieved baggage. Ideally, it will be our writable baggage because we had put it + // there in the context. + return Optional.of(HelidonOpenTelemetry.create(otelSpan, otelBaggage)); } @Override