Repository: helidon-io/helidon (java)

Issue:
Helidon 4.0.9 gets NPE on context aware wrapped async virtual thread use in JaegerDataPropagationProvider.java:41

## Environment Details
* Helidon Version: 4.0.9 SE
* JDK version: Graal 21
* OS: OL 7u5

----------

## Problem Description
Working tests in 4.0.8 are getting NPE in 4.0.9

[ERROR] validate.helidon.tracing.TracerPropagationTest.concurrentVirtualThreadUseCanary  Time elapsed: 0.02 s  <<< ERROR!
java.lang.NullPointerException: Cannot invoke "io.helidon.tracing.providers.opentelemetry.OpenTelemetryDataPropagationProvider$OpenTelemetryContext.tracer()" because "delegate" is null
	at io.helidon.tracing.providers.jaeger@4.0.9/io.helidon.tracing.providers.jaeger.JaegerDataPropagationProvider$JaegerContext.<init>(JaegerDataPropagationProvider.java:41)

(complete stack will be uploaded)

## Steps to reproduce
See uploaded test class that passes in 4.0.8 and fails in 4.0.9



Pull request:
Fix tracer information propagation across threads using Helidon context
### Description
Resolves #8824 

The earlier PR #8742 added propagation of tracer information for the OTel provider that was patterned after the long-standing Jaeger propagation. 

It turns out there was a flaw in that logic for choosing the span to propagate. Instead of using the current span as managed by the tracing implementation the code used whatever span, if any, that was already set in the current Helidon `Context` and actually ignored the tracing implementation's notion of the currently-active span. 

The key change here is that the propagation logic uses the span _from the tracing implementation's current span_ only. 

Any span that might already be set in the current context at the time propagation is being prepared is immaterial. For example, in the time since the span was set in that context some user code might have closed the scope which had made that span the current one, meaning no span was current. Or user code could have made some _other_ span the current span in the meantime.

The PR also contains a slight adaptation of a user test that had exposed this problem in the first place (see the attachment to the issue). The new test is in `provider-tests` so all provider implementations by default need to pass it. The test is written using the Helidon neutral tracing API. Because our Zipkin provider implementation does not implement the Helidon tracing API, this PR modifies the Zipkin build to exclude the new test. A separate issue will capture the possible change for Zipkin to implement the Helidon neutral tracing API at which point the new test from this PR should no longer be excluded from the Zipkin build.

### Documentation
Bug fix; no doc impact.

Patch:
diff --git a/tracing/providers/opentelemetry/src/main/java/io/helidon/tracing/providers/opentelemetry/OpenTelemetryDataPropagationProvider.java b/tracing/providers/opentelemetry/src/main/java/io/helidon/tracing/providers/opentelemetry/OpenTelemetryDataPropagationProvider.java
index 2c7c9561b3..014c1283c3 100644
--- a/tracing/providers/opentelemetry/src/main/java/io/helidon/tracing/providers/opentelemetry/OpenTelemetryDataPropagationProvider.java
+++ b/tracing/providers/opentelemetry/src/main/java/io/helidon/tracing/providers/opentelemetry/OpenTelemetryDataPropagationProvider.java
@@ -32,10 +32,15 @@ public class OpenTelemetryDataPropagationProvider
 
     @Override
     public OpenTelemetryContext data() {
-        return Contexts.context().map(context -> context.get(Span.class).map(span -> {
-            Tracer tracer = context.get(Tracer.class).orElseGet(OpenTelemetryTracerProvider::globalTracer);
-            return new OpenTelemetryContext(tracer, span);
-        }).orElse(null)).orElse(null);
+        // Use a tracer from the current context--because there is no notion in OTel of a "current" tracer--or from the
+        // global tracer.
+        Tracer tracer = Contexts.context().flatMap(ctx -> ctx.get(Tracer.class)).orElseGet(Tracer::global);
+
+        // Get the current span only from OTel's notion of the current span. We do not care what span might be set in the
+        // current context, because after that context was constructed user code could have closed the current span or set
+        // a new span as current.
+        Span span = OpenTelemetryTracerProvider.activeSpan().orElse(null);
+        return new OpenTelemetryContext(tracer, span);
     }
 
     @Override
@@ -51,7 +56,7 @@ public class OpenTelemetryDataPropagationProvider
 
     @Override
     public void propagateData(OpenTelemetryContext context) {
-        if (context != null) {
+        if (context != null && context.span != null) {
             context.scope = context.span.activate();
         }
     }
diff --git a/tracing/providers/zipkin/pom.xml b/tracing/providers/zipkin/pom.xml
index 2d0a706d4c..2369688362 100644
--- a/tracing/providers/zipkin/pom.xml
+++ b/tracing/providers/zipkin/pom.xml
@@ -115,6 +115,19 @@
                     </annotationProcessorPaths>
                 </configuration>
             </plugin>
+            <!--
+                Because the Helidon Zipkin provider does not itself implement the Helidon tracing API (it does so through
+                OpenTracing), exempt this build from a test that requires that.
+            -->
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-surefire-plugin</artifactId>
+                <configuration>
+                    <excludes>
+                        <exclude>io.helidon.tracing.providers.tests.TestTracerAndSpanPropagation.java</exclude>
+                    </excludes>
+                </configuration>
+            </plugin>
         </plugins>
     </build>
 </project>


Test patch:
diff --git a/tracing/provider-tests/src/main/java/io/helidon/tracing/providers/tests/TestTracerAndSpanPropagation.java b/tracing/provider-tests/src/main/java/io/helidon/tracing/providers/tests/TestTracerAndSpanPropagation.java
new file mode 100644
index 0000000000..32b9524f3e
--- /dev/null
+++ b/tracing/provider-tests/src/main/java/io/helidon/tracing/providers/tests/TestTracerAndSpanPropagation.java
@@ -0,0 +1,137 @@
+/*
+ * 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.providers.tests;
+
+import java.util.Objects;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import io.helidon.common.context.Context;
+import io.helidon.common.context.Contexts;
+import io.helidon.common.testing.junit5.OptionalMatcher;
+import io.helidon.tracing.Span;
+import io.helidon.tracing.SpanContext;
+import io.helidon.tracing.Tracer;
+
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.sameInstance;
+
+/**
+ * Tests that the current tracer and span are propagated through Helidon contexts.
+ * <p>
+ * Derived as closely as possible from a user-supplied reproducer. Basically, logging was changed to use system
+ * logging instead of SLF4J as in the original test and test assertions were converted to use Hamcrest. The key logic
+ * in the test is unchanged.
+ * </p>
+ */
+class TestTracerAndSpanPropagation {
+    private static final System.Logger LOGGER = System
+            .getLogger(TestTracerAndSpanPropagation.class.getName());
+
+    @Test
+    void concurrentVirtualThreadUseCanary() {
+        Contexts.runInContext(Context.create(), this::actualTest);
+    }
+
+    private void actualTest() {
+
+        final var tracer = buildTracer();
+        LOGGER.log(System.Logger.Level.INFO, "Tracer {0}", tracer);
+        assertThat("Tracer is enabled", tracer.enabled(), is(true));
+        assertThat("Tracer in use is the global tracer", tracer, sameInstance(Tracer.global()));
+        final var rootSpan = tracer.spanBuilder(getClass().getSimpleName()).start();
+        LOGGER.log(System.Logger.Level.INFO, "traceId: {0}", rootSpan.context().traceId());
+        try (var ignored = rootSpan.activate()) {
+            assertThat("rootSpan activate",
+                       Span.current().map(Span::context).map(SpanContext::spanId),
+                       OptionalMatcher.optionalValue(is(rootSpan.context().spanId())));
+
+            final var ff = new CompletableFuture[1];
+            try (var executor = Contexts.wrap(Executors.newVirtualThreadPerTaskExecutor())) {
+                final var futures = new CompletableFuture[5];
+                for (int i = 0; i < 5; i++) {
+                    futures[i] = CompletableFuture
+                            .runAsync(new ChildAction(tracer, rootSpan), executor);
+                }
+                for (final var f : futures) {
+                    ff[0] = f;
+                    f.get(1, TimeUnit.SECONDS);
+                }
+                LOGGER.log(System.Logger.Level.INFO, "all futures complete");
+
+            } catch (ExecutionException | TimeoutException | InterruptedException e) {
+                LOGGER.log(System.Logger.Level.ERROR, "Failure: f={0}", ff[0], e);
+                rootSpan.end(e);
+                throw new RuntimeException(e);
+            }
+            rootSpan.end();
+            LOGGER.log(System.Logger.Level.INFO, "ended rootSpan");
+        }
+        try {
+            Thread.sleep(100);
+        } catch (InterruptedException e) {
+            LOGGER.log(System.Logger.Level.ERROR, "Error on sleep", e);
+            throw new RuntimeException(e);
+        }
+        LOGGER.log(System.Logger.Level.INFO, "test ended");
+    }
+
+    private Tracer buildTracer() {
+        return Tracer.global();
+    }
+
+    record ChildAction(Tracer globalTracer, Span rootSpan) implements Runnable {
+
+        @Override
+        public void run() {
+            final var thread = Thread.currentThread();
+            assertThat("isVirtual false in ChildAction", thread.isVirtual(), is(true));
+            final var threadName = String.valueOf(thread);
+            LOGGER.log(System.Logger.Level.INFO, "Running {0}", threadName);
+
+            final var tracer = Objects.requireNonNull(Tracer.global(), "global NOT in ChildAction");
+            assertThat("tracer NOT preserved in ChildAction", tracer, is(sameInstance(globalTracer)));
+            assertThat("Current span from test NOT present in ChildAction",
+                       Span.current().map(Span::context).map(SpanContext::spanId),
+                       OptionalMatcher.optionalValue(is(rootSpan.context().spanId())));
+
+            final var span = Span.current().get();
+            final var spanContext = span.context();
+            final var childSpan = tracer.spanBuilder(threadName)
+                    .parent(spanContext).start();
+            try (var ignored = childSpan.activate()) {
+
+                assertThat("childSpan NOT activated",
+                           Span.current().map(Span::context).map(SpanContext::spanId),
+                           OptionalMatcher.optionalValue(is(childSpan.context().spanId())));
+                Thread.sleep(10);
+                span.end();
+
+            } catch (InterruptedException e) {
+                span.end(e);
+                throw new RuntimeException(e);
+            } finally {
+                LOGGER.log(System.Logger.Level.INFO, "Ended {0}", threadName);
+            }
+        }
+    }
+}
diff --git a/tracing/provider-tests/src/main/java/module-info.java b/tracing/provider-tests/src/main/java/module-info.java
index 8a90d89376..dd4c2b9701 100644
--- a/tracing/provider-tests/src/main/java/module-info.java
+++ b/tracing/provider-tests/src/main/java/module-info.java
@@ -21,6 +21,7 @@ module io.helidon.tracing.provider.tests {
 
     requires java.logging;
     requires io.helidon.tracing;
+    requires io.helidon.common.context;
     requires io.helidon.common.testing.junit5;
 
     requires org.junit.jupiter.api;