[3.x ] - Fix OpenTracingSpan Baggage propagation issue #6987

dalexandrov · 2023-06-12T14:15:56Z

Resolves #6970

trentjeff · 2023-06-14T14:22:14Z

...s/integration/gh-6970/src/test/java/io/helidon/tests/integration/tracerbaggage/MainTest.java

+        // Check baggage propagated
+        List<String> result = new ArrayList<>();
+        consumer.keys().forEach(result::add);
+        assertThat(result, hasItem(containsString("fubar")));


might be worth testing the value also

A stronger test would be to use Runtime to spawn another process with a ClientMain that calls into the server.

Added value test

spericas · 2023-06-14T14:34:33Z

tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetrySpan.java

@@ -118,6 +118,7 @@ public Optional<String> baggage(String key) {
    // Check if OTEL Context is already available in Global Helidon Context.
    // If not – use Current context.
    private static Context getContext() {
+        Context.current().makeCurrent();


Not an expert in the API, but this line gives me pause. We are making current something that is current? And the scope that is returned (which presumably needs closing at some point) is ignored? Can we explain this better?

Just a leftover. Now removed!

spericas · 2023-06-14T14:35:00Z

tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetrySpan.java

@@ -104,7 +104,7 @@ public Span baggage(String key, String value) {
        Baggage.builder()
                .put(key, value)
                .build()
-                .storeInContext(getContext())
+                .storeInContext(getContext().with(delegate))
                .makeCurrent();


Why is the resulting Scope ignored here?

RickyFrost · 2023-06-14T17:16:25Z

tracing/opentracing/src/main/java/io/helidon/tracing/opentracing/OpenTracingSpan.java

@@ -66,7 +64,7 @@ public void status(Status status) {

    @Override
    public SpanContext context() {
-        return context;
+        return Span.current().orElseThrow().context();


Why not return the wrapped delegate.context() here? (i.e. the line removed in above constructor). Aren't you unnecessarily risking a throw here?

This is outdated.

RickyFrost · 2023-06-14T17:19:46Z

tracing/opentracing/src/test/java/io/helidon/tracing/opentracing/BaggageTest.java

+            // Inject the span (context) into the consumer
+            final var consumer = HeaderConsumer
+                    .create(new TreeMap<>(String.CASE_INSENSITIVE_ORDER));
+            tracer.inject(Span.current().orElseThrow().context(), HeaderProvider.empty(), consumer);


Using Span.current()....context() here works around the issue of the bug. span.context() is needed here to really prove the problem has been fixed.

This is outdated.

RickyFrost · 2023-06-14T17:21:23Z

tracing/opentracing/src/test/java/io/helidon/tracing/opentracing/BaggageTest.java

+            tracer.inject(Span.current().orElseThrow().context(), HeaderProvider.empty(), consumer);
+
+
+            // Confirm that baggage was NOT propagated (the bug)


This comment was appropriate for the standalone test case showing the issue, but as a test for the fix remove 'NOT'

This is outdated.

RickyFrost · 2023-06-14T17:21:52Z

tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/BaggageTest.java

+                    .create(new TreeMap<>(String.CASE_INSENSITIVE_ORDER));
+            tracer.inject(span.context(), HeaderProvider.empty(), consumer);
+
+            // Confirm that baggage was NOT propagated (the bug)


This comment was appropriate for the standalone test case showing the issue, but as a test for the fix remove 'NOT'

This is outdated.

RickyFrost

See detail comments

dalexandrov · 2023-06-14T18:40:44Z

See detail comments

Looks like you have reviewed something outdated...

tests/integration/gh-6970/.dockerignore

tomas-langer · 2023-06-15T09:39:24Z

tests/integration/gh-6970/pom.xml

+                <artifactId>maven-surefire-plugin</artifactId>
+            </plugin>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>


Not really needed for atest (this is only useful when running from command line or packaging to docker)

not removed - comment was about copy libs

tests/integration/gh-6970/src/main/java/io/helidon/tests/integration/tracerbaggage/Main.java

tests/integration/gh-6970/src/main/resources/logging.properties

...s/integration/gh-6970/src/test/java/io/helidon/tests/integration/tracerbaggage/MainTest.java

tomas-langer · 2023-06-15T16:29:52Z

...s/integration/gh-6970/src/test/java/io/helidon/tests/integration/tracerbaggage/MainTest.java

+package io.helidon.tests.integration.tracerbaggage;
+
+import io.helidon.config.Config;
+import io.helidon.tracing.*;


star imports are not permitted. Please use our code style.

Something's wrong with my Idea after the update... Fixed!

tomas-langer · 2023-06-15T16:30:07Z

...s/integration/gh-6970/src/test/java/io/helidon/tests/integration/tracerbaggage/MainTest.java

+    public static final String VALUE = "1";
+
+    @BeforeAll
+    static void startTheServer() {


method name is wrong

tomas-langer · 2023-06-15T16:30:24Z

tests/integration/gh-6970/src/test/resources/application.yaml

+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+server:


server section is no longer needed

tomas-langer · 2023-06-15T16:31:01Z

tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetrySpan.java

@@ -104,7 +104,7 @@ public Span baggage(String key, String value) {
        Baggage.builder()
                .put(key, value)
                .build()
-                .storeInContext(getContext())
+                .storeInContext(getContext().with(delegate))


wrong formatting, please keep same code style (.with(delegate) should be on a new line)

tomas-langer · 2023-06-15T16:31:12Z

tests/integration/gh-6970/pom.xml

+    </properties>
+
+    <dependencies>
+        <dependency>


tomas-langer · 2023-06-15T16:31:18Z

tests/integration/gh-6970/pom.xml

+            <groupId>io.helidon.webserver</groupId>
+            <artifactId>helidon-webserver</artifactId>
+        </dependency>
+        <dependency>


tomas-langer · 2023-06-15T16:32:09Z

tests/integration/gh-6970/pom.xml

+                <artifactId>maven-surefire-plugin</artifactId>
+            </plugin>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>


not removed - comment was about copy libs

tomas-langer · 2023-06-15T16:32:54Z

tests/integration/gh-6970/pom.xml

+
+    <build>
+        <plugins>
+            <plugin>


not needed (probably the whole build section, if the tests are running)

RickyFrost · 2023-06-15T18:28:01Z

This fix is solving the wrong problem. See today's update in #6970

Fix OpenTracingSpan Baggage propagation issue

bfb1a5d

dalexandrov added tracing 3.x Issues for 3.x version branch telemetry labels Jun 12, 2023

dalexandrov self-assigned this Jun 12, 2023

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 12, 2023

dalexandrov added 7 commits June 12, 2023 17:38

Correct context usage for openTelemetery

13add8b

Move tests to integrations

edb1b55

Move tests to integrations

49b97d4

Finalize test

f1f7155

Finalize test

f75bed4

Finalize test

1438af8

Finalize test

b61547d

dalexandrov marked this pull request as ready for review June 14, 2023 11:57

trentjeff reviewed Jun 14, 2023

View reviewed changes

dalexandrov requested a review from tomas-langer June 14, 2023 14:30

spericas reviewed Jun 14, 2023

View reviewed changes

Address comments

faaa4ec

RickyFrost reviewed Jun 14, 2023

View reviewed changes

RickyFrost suggested changes Jun 14, 2023

View reviewed changes

trentjeff previously approved these changes Jun 14, 2023

View reviewed changes

tomas-langer requested changes Jun 15, 2023

View reviewed changes

Address Tomas comments

86cb069

dalexandrov dismissed trentjeff’s stale review via 86cb069 June 15, 2023 13:46

dalexandrov requested a review from tomas-langer June 15, 2023 13:47

tomas-langer requested changes Jun 15, 2023

View reviewed changes

Remove redundant stuff

23534ee

dalexandrov requested a review from tomas-langer June 15, 2023 16:52

tomas-langer approved these changes Jun 15, 2023

View reviewed changes

dalexandrov merged commit d785d8b into helidon-io:helidon-3.x Jun 15, 2023

dalexandrov deleted the 6970_OpenTracingSpan branch June 15, 2023 18:52

		tracer.inject(Span.current().orElseThrow().context(), HeaderProvider.empty(), consumer);


		// Confirm that baggage was NOT propagated (the bug)

[3.x ] - Fix OpenTracingSpan Baggage propagation issue #6987

[3.x ] - Fix OpenTracingSpan Baggage propagation issue #6987

Conversation

dalexandrov commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spericas Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RickyFrost left a comment

Choose a reason for hiding this comment

dalexandrov commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RickyFrost commented Jun 15, 2023

spericas Jun 14, 2023 •

edited

Loading