From be1fd69c6766af7a429dfc53ffd7fc7dd6fbf22b Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Thu, 23 Feb 2023 10:56:12 +0000 Subject: [PATCH 1/9] add MDC baggage --- .../mdc/v1_0/LoggingEventInstrumentation.java | 18 ++++++++++++ .../logback/v1_0/LogbackTest.groovy | 5 ++++ .../mdc/v1_0/OpenTelemetryAppender.java | 14 +++++++++ .../logback/mdc/v1_0/LogbackTest.groovy | 4 +++ .../mdc/v1_0/AbstractLogbackTest.groovy | 29 ++++++++++++++++--- .../bootstrap/Java8BytecodeBridge.java | 5 ++++ 6 files changed, 71 insertions(+), 4 deletions(-) diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java index aeb4ce31bd64..423b95d2cc4b 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java @@ -17,8 +17,11 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import ch.qos.logback.classic.spi.ILoggingEvent; +import io.opentelemetry.api.baggage.Baggage; +import io.opentelemetry.api.baggage.BaggageEntry; import io.opentelemetry.api.trace.SpanContext; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil; import io.opentelemetry.instrumentation.api.util.VirtualField; import io.opentelemetry.instrumentation.logback.mdc.v1_0.internal.UnionMap; import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge; @@ -54,11 +57,16 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class GetMdcAdvice { + // this has to be public otherwise the Advice cannot access it + // making this non-final helps greatly with testing + public static boolean addBaggage = + ConfigPropertiesUtil.getBoolean("otel.instrumentation.logback.mdc.add-baggage", false); @Advice.OnMethodExit(suppress = Throwable.class) public static void onExit( @Advice.This ILoggingEvent event, @Advice.Return(typing = Typing.DYNAMIC, readOnly = false) Map contextData) { + if (contextData != null && contextData.containsKey(TRACE_ID)) { // Assume already instrumented event if traceId is present. return; @@ -79,6 +87,16 @@ public static void onExit( spanContextData.put(SPAN_ID, spanContext.getSpanId()); spanContextData.put(TRACE_FLAGS, spanContext.getTraceFlags().asHex()); + if (addBaggage) { + Baggage baggage = Java8BytecodeBridge.baggageFromContext(context); + + // using a lambda here does not play nicely with instrumentation bytecode process + // (Java 6 related errors are observed) so relying on for loop instead + for (Map.Entry entry : baggage.asMap().entrySet()) { + spanContextData.put(entry.getKey(), entry.getValue().getValue()); + } + } + if (contextData == null) { contextData = spanContextData; } else { diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackTest.groovy b/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackTest.groovy index a666d5f99a91..a185901a8285 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackTest.groovy @@ -7,6 +7,11 @@ package io.opentelemetry.javaagent.instrumentation.logback.v1_0 import io.opentelemetry.instrumentation.logback.mdc.v1_0.AbstractLogbackTest import io.opentelemetry.instrumentation.test.AgentTestTrait +import io.opentelemetry.javaagent.instrumentation.logback.mdc.v1_0.LoggingEventInstrumentation class LogbackTest extends AbstractLogbackTest implements AgentTestTrait { + @Override + void setBaggageFlag() { + LoggingEventInstrumentation.GetMdcAdvice.addBaggage = false + } } diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java index caf4ea356786..bc51dd13b15f 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java +++ b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java @@ -14,8 +14,10 @@ import ch.qos.logback.core.UnsynchronizedAppenderBase; import ch.qos.logback.core.spi.AppenderAttachable; import ch.qos.logback.core.spi.AppenderAttachableImpl; +import io.opentelemetry.api.baggage.Baggage; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanContext; +import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil; import io.opentelemetry.instrumentation.logback.mdc.v1_0.internal.UnionMap; import java.util.HashMap; import java.util.Iterator; @@ -23,6 +25,13 @@ public class OpenTelemetryAppender extends UnsynchronizedAppenderBase implements AppenderAttachable { + private static boolean addBaggage = + ConfigPropertiesUtil.getBoolean("otel.instrumentation.logback.mdc.add-baggage", false); + + // to aid testing + public static void setAddBaggage(boolean addBaggage) { + OpenTelemetryAppender.addBaggage = addBaggage; + } private final AppenderAttachableImpl aai = new AppenderAttachableImpl<>(); @@ -44,6 +53,11 @@ public static ILoggingEvent wrapEvent(ILoggingEvent event) { contextData.put(SPAN_ID, spanContext.getSpanId()); contextData.put(TRACE_FLAGS, spanContext.getTraceFlags().asHex()); + if (addBaggage) { + Baggage baggage = Baggage.current(); + baggage.forEach((key, value) -> contextData.put(key, value.getValue())); + } + if (eventContext == null) { eventContext = contextData; } else { diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.groovy b/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.groovy index 3bd3f72f39cc..b68831f2040b 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.groovy @@ -8,4 +8,8 @@ package io.opentelemetry.instrumentation.logback.mdc.v1_0 import io.opentelemetry.instrumentation.test.LibraryTestTrait class LogbackTest extends AbstractLogbackTest implements LibraryTestTrait { + @Override + void setBaggageFlag() { + OpenTelemetryAppender.addBaggage = false + } } diff --git a/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy b/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy index 791f916332c1..69d25b39baf3 100644 --- a/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy @@ -7,6 +7,7 @@ package io.opentelemetry.instrumentation.logback.mdc.v1_0 import ch.qos.logback.classic.spi.ILoggingEvent import ch.qos.logback.core.read.ListAppender +import io.opentelemetry.api.baggage.Baggage import io.opentelemetry.api.trace.Span import io.opentelemetry.instrumentation.test.InstrumentationSpecification import org.slf4j.Logger @@ -35,6 +36,7 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { def setup() { listAppender.list.clear() + setBaggageFlag() } def "no ids when no span"() { @@ -59,16 +61,16 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { def "ids when span"() { when: - Span span1 = runWithSpan("test") { + Baggage baggage = Baggage.empty().toBuilder().put("baggage_key", "baggage_value").build() + + Span span1 = runWithSpanAndBaggage("test", baggage) { AbstractLogbackTest.logger.info("log message 1") - Span.current() } logger.info("log message 2") - Span span2 = runWithSpan("test 2") { + Span span2 = runWithSpanAndBaggage("test 2", baggage) { AbstractLogbackTest.logger.info("log message 3") - Span.current() } def events = listAppender.list @@ -79,15 +81,34 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { events[0].getMDCPropertyMap().get("trace_id") == span1.spanContext.traceId events[0].getMDCPropertyMap().get("span_id") == span1.spanContext.spanId events[0].getMDCPropertyMap().get("trace_flags") == "01" + events[0].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage_value" : null) events[1].message == "log message 2" events[1].getMDCPropertyMap().get("trace_id") == null events[1].getMDCPropertyMap().get("span_id") == null events[1].getMDCPropertyMap().get("trace_flags") == null + events[1].getMDCPropertyMap().get("baggage_key") == null events[2].message == "log message 3" events[2].getMDCPropertyMap().get("trace_id") == span2.spanContext.traceId events[2].getMDCPropertyMap().get("span_id") == span2.spanContext.spanId events[2].getMDCPropertyMap().get("trace_flags") == "01" + events[2].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage_value" : null) + } + + Span runWithSpanAndBaggage(String spanName, Baggage baggage, Closure callback) { + return runWithSpan(spanName) { + try (var unusedScope = baggage.makeCurrent()) { + callback.call() + } + + Span.current() + } + } + + abstract void setBaggageFlag() + + boolean expectBaggage() { + return false } } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/bootstrap/Java8BytecodeBridge.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/bootstrap/Java8BytecodeBridge.java index 52ed8541de37..588b48e23916 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/bootstrap/Java8BytecodeBridge.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/bootstrap/Java8BytecodeBridge.java @@ -5,6 +5,7 @@ package io.opentelemetry.javaagent.bootstrap; +import io.opentelemetry.api.baggage.Baggage; import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; @@ -37,5 +38,9 @@ public static Span spanFromContext(Context context) { return Span.fromContext(context); } + public static Baggage baggageFromContext(Context context) { + return Baggage.fromContext(context); + } + private Java8BytecodeBridge() {} } From 8072829feff5fa4a9706939b5120756e378753be Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Thu, 23 Feb 2023 10:59:56 +0000 Subject: [PATCH 2/9] add missing files --- .../logback/v1_0/LogbackWithBaggageTest.groovy | 18 ++++++++++++++++++ .../mdc/v1_0/LogbackWithBaggageTest.groovy | 16 ++++++++++++++++ .../v1_0/AbstractLogbackWithBaggageTest.groovy | 13 +++++++++++++ 3 files changed, 47 insertions(+) create mode 100644 instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy create mode 100644 instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy create mode 100644 instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackWithBaggageTest.groovy diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy b/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy new file mode 100644 index 000000000000..df609100195d --- /dev/null +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy @@ -0,0 +1,18 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.logback.v1_0 + +import io.opentelemetry.instrumentation.logback.mdc.v1_0.AbstractLogbackWithBaggageTest +import io.opentelemetry.instrumentation.test.AgentTestTrait +import io.opentelemetry.javaagent.instrumentation.logback.mdc.v1_0.LoggingEventInstrumentation + +class LogbackWithBaggageTest extends AbstractLogbackWithBaggageTest implements AgentTestTrait { + + @Override + void setBaggageFlag() { + LoggingEventInstrumentation.GetMdcAdvice.addBaggage = true + } +} diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy b/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy new file mode 100644 index 000000000000..80452cb11627 --- /dev/null +++ b/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy @@ -0,0 +1,16 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.logback.mdc.v1_0 + + +import io.opentelemetry.instrumentation.test.LibraryTestTrait + +class LogbackWithBaggageTest extends AbstractLogbackWithBaggageTest implements LibraryTestTrait { + @Override + void setBaggageFlag() { + OpenTelemetryAppender.addBaggage = true + } +} diff --git a/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackWithBaggageTest.groovy b/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackWithBaggageTest.groovy new file mode 100644 index 000000000000..66762efeac06 --- /dev/null +++ b/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackWithBaggageTest.groovy @@ -0,0 +1,13 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.logback.mdc.v1_0 + +abstract class AbstractLogbackWithBaggageTest extends AbstractLogbackTest { + @Override + boolean expectBaggage() { + return true + } +} From 6ac2663e53543f6726710c2b0193b35841c5c41f Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Fri, 24 Feb 2023 13:50:46 +0000 Subject: [PATCH 3/9] refactor based on review comments --- .../javaagent/build.gradle.kts | 19 ++++++++++++ .../v1_0/LogbackWithBaggageTest.groovy | 6 ---- .../src/addBaggageTest/resources/logback.xml | 19 ++++++++++++ .../logback/mdc/v1_0/LogbackSingletons.java | 19 ++++++++++++ .../mdc/v1_0/LoggingEventInstrumentation.java | 22 +++++-------- .../logback/v1_0/LogbackTest.groovy | 5 --- .../logback-mdc-1.0/library/build.gradle.kts | 13 ++++++++ .../mdc/v1_0/LogbackWithBaggageTest.groovy | 4 --- .../src/addBaggageTest/resources/logback.xml | 31 +++++++++++++++++++ .../mdc/v1_0/OpenTelemetryAppender.java | 30 ++++++++---------- .../logback/mdc/v1_0/LogbackTest.groovy | 4 --- .../mdc/v1_0/AbstractLogbackTest.groovy | 22 ++++++++----- 12 files changed, 136 insertions(+), 58 deletions(-) rename instrumentation/logback/logback-mdc-1.0/javaagent/src/{test => addBaggageTest}/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy (66%) create mode 100644 instrumentation/logback/logback-mdc-1.0/javaagent/src/addBaggageTest/resources/logback.xml create mode 100644 instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java rename instrumentation/logback/logback-mdc-1.0/library/src/{test => addBaggageTest}/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy (78%) create mode 100644 instrumentation/logback/logback-mdc-1.0/library/src/addBaggageTest/resources/logback.xml diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts b/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts index 5c315ed3394f..2a4f862e6064 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts @@ -1,5 +1,6 @@ plugins { id("otel.javaagent-instrumentation") + id("org.unbroken-dome.test-sets") } muzzle { @@ -10,6 +11,10 @@ muzzle { } } +testSets { + create("addBaggageTest") +} + dependencies { implementation(project(":instrumentation:logback:logback-mdc-1.0:library")) @@ -42,3 +47,17 @@ dependencies { testImplementation(project(":instrumentation:logback:logback-mdc-1.0:testing")) } + +tasks { + val addBaggageTest by existing(Test::class) { + jvmArgs("-Dotel.instrumentation.logback.mdc.add-baggage=true") + } + + test { + jvmArgs("-Dotel.instrumentation.logback.mdc.add-baggage=false") + } + + named("check") { + dependsOn(addBaggageTest) + } +} diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy b/instrumentation/logback/logback-mdc-1.0/javaagent/src/addBaggageTest/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy similarity index 66% rename from instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy rename to instrumentation/logback/logback-mdc-1.0/javaagent/src/addBaggageTest/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy index df609100195d..e2468e46cc9a 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/addBaggageTest/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackWithBaggageTest.groovy @@ -7,12 +7,6 @@ package io.opentelemetry.javaagent.instrumentation.logback.v1_0 import io.opentelemetry.instrumentation.logback.mdc.v1_0.AbstractLogbackWithBaggageTest import io.opentelemetry.instrumentation.test.AgentTestTrait -import io.opentelemetry.javaagent.instrumentation.logback.mdc.v1_0.LoggingEventInstrumentation class LogbackWithBaggageTest extends AbstractLogbackWithBaggageTest implements AgentTestTrait { - - @Override - void setBaggageFlag() { - LoggingEventInstrumentation.GetMdcAdvice.addBaggage = true - } } diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/addBaggageTest/resources/logback.xml b/instrumentation/logback/logback-mdc-1.0/javaagent/src/addBaggageTest/resources/logback.xml new file mode 100644 index 000000000000..3434fbaaab59 --- /dev/null +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/addBaggageTest/resources/logback.xml @@ -0,0 +1,19 @@ + + + + + + + %coloredLevel %logger{15} - %message%n%xException{10} + + + + + + + + + + + + diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java new file mode 100644 index 000000000000..73a28a0d355e --- /dev/null +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java @@ -0,0 +1,19 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.logback.mdc.v1_0; + +import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil; + +public final class LogbackSingletons { + private static final boolean ADD_BAGGAGE = + ConfigPropertiesUtil.getBoolean("otel.instrumentation.logback.mdc.add-baggage", false); + + public static boolean addBaggage() { + return ADD_BAGGAGE; + } + + private LogbackSingletons() {} +} diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java index 423b95d2cc4b..f2375cbb39fe 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java @@ -21,7 +21,6 @@ import io.opentelemetry.api.baggage.BaggageEntry; import io.opentelemetry.api.trace.SpanContext; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil; import io.opentelemetry.instrumentation.api.util.VirtualField; import io.opentelemetry.instrumentation.logback.mdc.v1_0.internal.UnionMap; import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge; @@ -57,11 +56,6 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class GetMdcAdvice { - // this has to be public otherwise the Advice cannot access it - // making this non-final helps greatly with testing - public static boolean addBaggage = - ConfigPropertiesUtil.getBoolean("otel.instrumentation.logback.mdc.add-baggage", false); - @Advice.OnMethodExit(suppress = Throwable.class) public static void onExit( @Advice.This ILoggingEvent event, @@ -77,17 +71,17 @@ public static void onExit( return; } + Map spanContextData = new HashMap<>(); + SpanContext spanContext = Java8BytecodeBridge.spanFromContext(context).getSpanContext(); - if (!spanContext.isValid()) { - return; - } - Map spanContextData = new HashMap<>(); - spanContextData.put(TRACE_ID, spanContext.getTraceId()); - spanContextData.put(SPAN_ID, spanContext.getSpanId()); - spanContextData.put(TRACE_FLAGS, spanContext.getTraceFlags().asHex()); + if (spanContext.isValid()) { + spanContextData.put(TRACE_ID, spanContext.getTraceId()); + spanContextData.put(SPAN_ID, spanContext.getSpanId()); + spanContextData.put(TRACE_FLAGS, spanContext.getTraceFlags().asHex()); + } - if (addBaggage) { + if (LogbackSingletons.addBaggage()) { Baggage baggage = Java8BytecodeBridge.baggageFromContext(context); // using a lambda here does not play nicely with instrumentation bytecode process diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackTest.groovy b/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackTest.groovy index a185901a8285..a666d5f99a91 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/logback/v1_0/LogbackTest.groovy @@ -7,11 +7,6 @@ package io.opentelemetry.javaagent.instrumentation.logback.v1_0 import io.opentelemetry.instrumentation.logback.mdc.v1_0.AbstractLogbackTest import io.opentelemetry.instrumentation.test.AgentTestTrait -import io.opentelemetry.javaagent.instrumentation.logback.mdc.v1_0.LoggingEventInstrumentation class LogbackTest extends AbstractLogbackTest implements AgentTestTrait { - @Override - void setBaggageFlag() { - LoggingEventInstrumentation.GetMdcAdvice.addBaggage = false - } } diff --git a/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts b/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts index 6bf9d8c2bf2e..c619dcac0831 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts +++ b/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts @@ -1,5 +1,10 @@ plugins { id("otel.library-instrumentation") + id("org.unbroken-dome.test-sets") +} + +testSets { + create("addBaggageTest") } dependencies { @@ -32,3 +37,11 @@ dependencies { testImplementation(project(":instrumentation:logback:logback-mdc-1.0:testing")) } + +tasks { + val addBaggageTest by existing + + named("check") { + dependsOn(addBaggageTest) + } +} diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy b/instrumentation/logback/logback-mdc-1.0/library/src/addBaggageTest/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy similarity index 78% rename from instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy rename to instrumentation/logback/logback-mdc-1.0/library/src/addBaggageTest/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy index 80452cb11627..b1ddcc750212 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/library/src/addBaggageTest/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackWithBaggageTest.groovy @@ -9,8 +9,4 @@ package io.opentelemetry.instrumentation.logback.mdc.v1_0 import io.opentelemetry.instrumentation.test.LibraryTestTrait class LogbackWithBaggageTest extends AbstractLogbackWithBaggageTest implements LibraryTestTrait { - @Override - void setBaggageFlag() { - OpenTelemetryAppender.addBaggage = true - } } diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/addBaggageTest/resources/logback.xml b/instrumentation/logback/logback-mdc-1.0/library/src/addBaggageTest/resources/logback.xml new file mode 100644 index 000000000000..6d7db9d2cc18 --- /dev/null +++ b/instrumentation/logback/logback-mdc-1.0/library/src/addBaggageTest/resources/logback.xml @@ -0,0 +1,31 @@ + + + + + + + + true + + + + + + + + diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java index bc51dd13b15f..e4c0e7389cfa 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java +++ b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java @@ -17,7 +17,6 @@ import io.opentelemetry.api.baggage.Baggage; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanContext; -import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil; import io.opentelemetry.instrumentation.logback.mdc.v1_0.internal.UnionMap; import java.util.HashMap; import java.util.Iterator; @@ -25,22 +24,15 @@ public class OpenTelemetryAppender extends UnsynchronizedAppenderBase implements AppenderAttachable { - private static boolean addBaggage = - ConfigPropertiesUtil.getBoolean("otel.instrumentation.logback.mdc.add-baggage", false); - - // to aid testing - public static void setAddBaggage(boolean addBaggage) { - OpenTelemetryAppender.addBaggage = addBaggage; - } + private volatile boolean addBaggage; private final AppenderAttachableImpl aai = new AppenderAttachableImpl<>(); - public static ILoggingEvent wrapEvent(ILoggingEvent event) { - Span currentSpan = Span.current(); - if (!currentSpan.getSpanContext().isValid()) { - return event; - } + public void setAddBaggage(boolean addBaggage) { + this.addBaggage = addBaggage; + } + public ILoggingEvent wrapEvent(ILoggingEvent event) { Map eventContext = event.getMDCPropertyMap(); if (eventContext != null && eventContext.containsKey(TRACE_ID)) { // Assume already instrumented event if traceId is present. @@ -48,10 +40,14 @@ public static ILoggingEvent wrapEvent(ILoggingEvent event) { } Map contextData = new HashMap<>(); - SpanContext spanContext = currentSpan.getSpanContext(); - contextData.put(TRACE_ID, spanContext.getTraceId()); - contextData.put(SPAN_ID, spanContext.getSpanId()); - contextData.put(TRACE_FLAGS, spanContext.getTraceFlags().asHex()); + Span currentSpan = Span.current(); + + if (currentSpan.getSpanContext().isValid()) { + SpanContext spanContext = currentSpan.getSpanContext(); + contextData.put(TRACE_ID, spanContext.getTraceId()); + contextData.put(SPAN_ID, spanContext.getSpanId()); + contextData.put(TRACE_FLAGS, spanContext.getTraceFlags().asHex()); + } if (addBaggage) { Baggage baggage = Baggage.current(); diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.groovy b/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.groovy index b68831f2040b..3bd3f72f39cc 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/library/src/test/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.groovy @@ -8,8 +8,4 @@ package io.opentelemetry.instrumentation.logback.mdc.v1_0 import io.opentelemetry.instrumentation.test.LibraryTestTrait class LogbackTest extends AbstractLogbackTest implements LibraryTestTrait { - @Override - void setBaggageFlag() { - OpenTelemetryAppender.addBaggage = false - } } diff --git a/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy b/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy index 69d25b39baf3..2c34a6a1e721 100644 --- a/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy @@ -36,13 +36,16 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { def setup() { listAppender.list.clear() - setBaggageFlag() } def "no ids when no span"() { when: - logger.info("log message 1") - logger.info("log message 2") + Baggage baggage = Baggage.empty().toBuilder().put("baggage_key", "baggage_value").build() + + runWithBaggage(baggage) { + AbstractLogbackTest.logger.info("log message 1") + AbstractLogbackTest.logger.info("log message 2") + } def events = listAppender.list @@ -52,11 +55,13 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { events[0].getMDCPropertyMap().get("trace_id") == null events[0].getMDCPropertyMap().get("span_id") == null events[0].getMDCPropertyMap().get("trace_flags") == null + events[0].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage_value" : null) events[1].message == "log message 2" events[1].getMDCPropertyMap().get("trace_id") == null events[1].getMDCPropertyMap().get("span_id") == null events[1].getMDCPropertyMap().get("trace_flags") == null + events[1].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage_value" : null) } def "ids when span"() { @@ -98,15 +103,16 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { Span runWithSpanAndBaggage(String spanName, Baggage baggage, Closure callback) { return runWithSpan(spanName) { - try (var unusedScope = baggage.makeCurrent()) { - callback.call() - } - + runWithBaggage(baggage, callback) Span.current() } } - abstract void setBaggageFlag() + void runWithBaggage(Baggage baggage, Closure callback) { + try (var unusedScope = baggage.makeCurrent()) { + callback.call() + } + } boolean expectBaggage() { return false From b06506a16ad6bfc4bd43c543a31df28e9ea16748 Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Fri, 24 Feb 2023 17:33:00 +0000 Subject: [PATCH 4/9] Update instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java Co-authored-by: Mateusz Rzeszutek --- .../instrumentation/logback/mdc/v1_0/LogbackSingletons.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java index 73a28a0d355e..a14cac81706e 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LogbackSingletons.java @@ -9,7 +9,7 @@ public final class LogbackSingletons { private static final boolean ADD_BAGGAGE = - ConfigPropertiesUtil.getBoolean("otel.instrumentation.logback.mdc.add-baggage", false); + ConfigPropertiesUtil.getBoolean("otel.instrumentation.logback-mdc.add-baggage", false); public static boolean addBaggage() { return ADD_BAGGAGE; From 86b354b4ae2920e61e7f66f39bd44435c50e78ee Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Sun, 26 Feb 2023 07:20:08 +0000 Subject: [PATCH 5/9] address comments --- .../logback/mdc/v1_0/OpenTelemetryAppender.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java index e4c0e7389cfa..e3b55515f732 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java +++ b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java @@ -17,6 +17,7 @@ import io.opentelemetry.api.baggage.Baggage; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanContext; +import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.logback.mdc.v1_0.internal.UnionMap; import java.util.HashMap; import java.util.Iterator; @@ -28,6 +29,12 @@ public class OpenTelemetryAppender extends UnsynchronizedAppenderBase aai = new AppenderAttachableImpl<>(); + /** + * When set to true this will enable addition of all baggage entries to MDC. This can be done by + * adding the following to the logback.xml config. + * {@code true} + * @param addBaggage True if baggage should be added to MDC + */ public void setAddBaggage(boolean addBaggage) { this.addBaggage = addBaggage; } @@ -40,7 +47,8 @@ public ILoggingEvent wrapEvent(ILoggingEvent event) { } Map contextData = new HashMap<>(); - Span currentSpan = Span.current(); + Context context = Context.current(); + Span currentSpan = Span.fromContext(context); if (currentSpan.getSpanContext().isValid()) { SpanContext spanContext = currentSpan.getSpanContext(); @@ -50,7 +58,7 @@ public ILoggingEvent wrapEvent(ILoggingEvent event) { } if (addBaggage) { - Baggage baggage = Baggage.current(); + Baggage baggage = Baggage.fromContext(context); baggage.forEach((key, value) -> contextData.put(key, value.getValue())); } From 7a252aea5ac22aa565e62b44a913ebb5570a4af7 Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Sun, 26 Feb 2023 07:37:46 +0000 Subject: [PATCH 6/9] fix build --- .../logback/logback-mdc-1.0/javaagent/build.gradle.kts | 4 ++-- .../logback/mdc/v1_0/OpenTelemetryAppender.java | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts b/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts index 2a4f862e6064..49a7dc9202a5 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts @@ -50,11 +50,11 @@ dependencies { tasks { val addBaggageTest by existing(Test::class) { - jvmArgs("-Dotel.instrumentation.logback.mdc.add-baggage=true") + jvmArgs("-Dotel.instrumentation.logback-mdc.add-baggage=true") } test { - jvmArgs("-Dotel.instrumentation.logback.mdc.add-baggage=false") + jvmArgs("-Dotel.instrumentation.logback-mdc.add-baggage=false") } named("check") { diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java index e3b55515f732..c1b9ec17f3c3 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java +++ b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java @@ -31,8 +31,9 @@ public class OpenTelemetryAppender extends UnsynchronizedAppenderBasetrue} + * adding the following to the logback.xml config for this appender. {@code + * true} + * * @param addBaggage True if baggage should be added to MDC */ public void setAddBaggage(boolean addBaggage) { From c82f1b3ed23c3caaa9c0e7fe12660d8e6cada6ae Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Wed, 8 Mar 2023 11:27:33 +0000 Subject: [PATCH 7/9] add baggage prefix to MDC values --- .../logback/mdc/v1_0/LoggingEventInstrumentation.java | 5 ++++- .../logback/mdc/v1_0/OpenTelemetryAppender.java | 7 ++++++- .../logback/mdc/v1_0/AbstractLogbackTest.groovy | 8 ++++---- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java index f2375cbb39fe..e2f8130e9406 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java @@ -87,7 +87,10 @@ public static void onExit( // using a lambda here does not play nicely with instrumentation bytecode process // (Java 6 related errors are observed) so relying on for loop instead for (Map.Entry entry : baggage.asMap().entrySet()) { - spanContextData.put(entry.getKey(), entry.getValue().getValue()); + spanContextData.put( + entry.getKey(), + // prefix all baggage values to avoid clashes with existing context + "baggage." + entry.getValue().getValue()); } } diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java index c1b9ec17f3c3..fdeea50ba3ba 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java +++ b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java @@ -60,7 +60,12 @@ public ILoggingEvent wrapEvent(ILoggingEvent event) { if (addBaggage) { Baggage baggage = Baggage.fromContext(context); - baggage.forEach((key, value) -> contextData.put(key, value.getValue())); + baggage.forEach( + (key, value) -> + contextData.put( + key, + // prefix all baggage values to avoid clashes with existing context + "baggage." + value.getValue())); } if (eventContext == null) { diff --git a/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy b/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy index 2c34a6a1e721..0062ea09bc67 100644 --- a/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy +++ b/instrumentation/logback/logback-mdc-1.0/testing/src/main/groovy/io/opentelemetry/instrumentation/logback/mdc/v1_0/AbstractLogbackTest.groovy @@ -55,13 +55,13 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { events[0].getMDCPropertyMap().get("trace_id") == null events[0].getMDCPropertyMap().get("span_id") == null events[0].getMDCPropertyMap().get("trace_flags") == null - events[0].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage_value" : null) + events[0].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage.baggage_value" : null) events[1].message == "log message 2" events[1].getMDCPropertyMap().get("trace_id") == null events[1].getMDCPropertyMap().get("span_id") == null events[1].getMDCPropertyMap().get("trace_flags") == null - events[1].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage_value" : null) + events[1].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage.baggage_value" : null) } def "ids when span"() { @@ -86,7 +86,7 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { events[0].getMDCPropertyMap().get("trace_id") == span1.spanContext.traceId events[0].getMDCPropertyMap().get("span_id") == span1.spanContext.spanId events[0].getMDCPropertyMap().get("trace_flags") == "01" - events[0].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage_value" : null) + events[0].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage.baggage_value" : null) events[1].message == "log message 2" events[1].getMDCPropertyMap().get("trace_id") == null @@ -98,7 +98,7 @@ abstract class AbstractLogbackTest extends InstrumentationSpecification { events[2].getMDCPropertyMap().get("trace_id") == span2.spanContext.traceId events[2].getMDCPropertyMap().get("span_id") == span2.spanContext.spanId events[2].getMDCPropertyMap().get("trace_flags") == "01" - events[2].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage_value" : null) + events[2].getMDCPropertyMap().get("baggage_key") == (expectBaggage() ? "baggage.baggage_value" : null) } Span runWithSpanAndBaggage(String spanName, Baggage baggage, Closure callback) { From fb2d59b94617a7f6ee4981170d976eadc0ad01ac Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Wed, 8 Mar 2023 16:31:49 +0000 Subject: [PATCH 8/9] use test suites --- .../javaagent/build.gradle.kts | 31 ++++++++++++------- .../logback-mdc-1.0/library/build.gradle.kts | 25 +++++++++++---- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts b/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts index 49a7dc9202a5..6ec1121a8eca 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts @@ -1,6 +1,5 @@ plugins { id("otel.javaagent-instrumentation") - id("org.unbroken-dome.test-sets") } muzzle { @@ -11,8 +10,24 @@ muzzle { } } -testSets { - create("addBaggageTest") +testing { + suites { + val addBaggageTest by registering(JvmTestSuite::class) { + targets { + all { + testTask.configure { + jvmArgs("-Dotel.instrumentation.logback-mdc.add-baggage=true") + } + } + } + } + } +} + +configurations { + named("addBaggageTestImplementation") { + extendsFrom(configurations["testImplementation"]) + } } dependencies { @@ -49,15 +64,7 @@ dependencies { } tasks { - val addBaggageTest by existing(Test::class) { - jvmArgs("-Dotel.instrumentation.logback-mdc.add-baggage=true") - } - - test { - jvmArgs("-Dotel.instrumentation.logback-mdc.add-baggage=false") - } - named("check") { - dependsOn(addBaggageTest) + dependsOn(testing.suites) } } diff --git a/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts b/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts index c619dcac0831..8c6ab07fdbed 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts +++ b/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts @@ -1,10 +1,25 @@ plugins { id("otel.library-instrumentation") - id("org.unbroken-dome.test-sets") } -testSets { - create("addBaggageTest") +testing { + suites { + val addBaggageTest by registering(JvmTestSuite::class) { + targets { + all { + testTask.configure { + jvmArgs("-Dotel.instrumentation.logback-mdc.add-baggage=true") + } + } + } + } + } +} + +configurations { + named("addBaggageTestImplementation") { + extendsFrom(configurations["testImplementation"]) + } } dependencies { @@ -39,9 +54,7 @@ dependencies { } tasks { - val addBaggageTest by existing - named("check") { - dependsOn(addBaggageTest) + dependsOn(testing.suites) } } From 3cc0ab85b0e59a20633025aea91fd0a575053b5a Mon Sep 17 00:00:00 2001 From: adamleantech <65166709+adamleantech@users.noreply.github.com> Date: Thu, 9 Mar 2023 10:41:05 +0000 Subject: [PATCH 9/9] fix test suites --- .../javaagent/build.gradle.kts | 43 +++++++++--------- .../logback-mdc-1.0/library/build.gradle.kts | 44 +++++++++---------- 2 files changed, 43 insertions(+), 44 deletions(-) diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts b/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts index 6ec1121a8eca..9d616d079f9a 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts +++ b/instrumentation/logback/logback-mdc-1.0/javaagent/build.gradle.kts @@ -21,12 +21,28 @@ testing { } } } - } -} -configurations { - named("addBaggageTestImplementation") { - extendsFrom(configurations["testImplementation"]) + withType(JvmTestSuite::class) { + dependencies { + if (findProperty("testLatestDeps") as Boolean) { + implementation("ch.qos.logback:logback-classic:+") + } else { + implementation("ch.qos.logback:logback-classic") { + version { + strictly("1.0.0") + } + } + implementation("org.slf4j:slf4j-api") { + version { + strictly("1.6.4") + } + } + } + + implementation(project(":instrumentation:logback:logback-mdc-1.0:testing")) + implementation(project(":instrumentation:logback:logback-mdc-1.0:javaagent")) + } + } } } @@ -44,23 +60,6 @@ dependencies { strictly("1.6.4") } } - - if (findProperty("testLatestDeps") as Boolean) { - testImplementation("ch.qos.logback:logback-classic:+") - } else { - testImplementation("ch.qos.logback:logback-classic") { - version { - strictly("1.0.0") - } - } - testImplementation("org.slf4j:slf4j-api") { - version { - strictly("1.6.4") - } - } - } - - testImplementation(project(":instrumentation:logback:logback-mdc-1.0:testing")) } tasks { diff --git a/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts b/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts index 8c6ab07fdbed..76537740d178 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts +++ b/instrumentation/logback/logback-mdc-1.0/library/build.gradle.kts @@ -13,16 +13,33 @@ testing { } } } - } -} -configurations { - named("addBaggageTestImplementation") { - extendsFrom(configurations["testImplementation"]) + withType(JvmTestSuite::class) { + dependencies { + if (findProperty("testLatestDeps") as Boolean) { + implementation("ch.qos.logback:logback-classic:+") + } else { + implementation("ch.qos.logback:logback-classic") { + version { + strictly("1.0.0") + } + } + implementation("org.slf4j:slf4j-api") { + version { + strictly("1.6.4") + } + } + } + + implementation(project(":instrumentation:logback:logback-mdc-1.0:testing")) + implementation(project(":instrumentation:logback:logback-mdc-1.0:library")) + } + } } } dependencies { + // pin the version strictly to avoid overriding by dependencyManagement versions compileOnly("ch.qos.logback:logback-classic") { version { @@ -34,23 +51,6 @@ dependencies { strictly("1.6.4") } } - - if (findProperty("testLatestDeps") as Boolean) { - testImplementation("ch.qos.logback:logback-classic:+") - } else { - testImplementation("ch.qos.logback:logback-classic") { - version { - strictly("1.0.0") - } - } - testImplementation("org.slf4j:slf4j-api") { - version { - strictly("1.6.4") - } - } - } - - testImplementation(project(":instrumentation:logback:logback-mdc-1.0:testing")) } tasks {