Add `service.name` to MDC #9647

cleverchuk · 2023-10-10T17:19:17Z

(#9297) add new instrumentation config otel.instrumentation.mdc.resource-attributes that will be used to configure Resource attributes to be added to logging map diagnostic context.

linux-foundation-easycla · 2023-10-10T17:19:23Z

The committers listed above are authorized under a signed CLA.

✅ login: cleverchuk (32e4a29, a0bf776, f4fb878, 153b74b, d5620f8, 9cf2689)
✅ login: laurit / name: Lauri Tulmin (740ef8a, 4e80330, 22fde67, 822f0b1)

laurit · 2023-10-11T05:46:32Z

Test fails because it uses a type that is not available due to which instrumentation is not applied

2023-10-10T21:01:07.5279882Z     [otel.javaagent 2023-10-10 21:01:05:834 +0000] [Test worker] WARN io.opentelemetry.javaagent.tooling.instrumentation.MuzzleMatcher - Instrumentation skipped, mismatched references were found: logback-mdc [class io.opentelemetry.javaagent.instrumentation.logback.mdc.v1_0.LogbackMdcInstrumentationModule] on sun.misc.Launcher$AppClassLoader@73d16e93
2023-10-10T21:01:07.5282284Z     [otel.javaagent 2023-10-10 21:01:05:834 +0000] [Test worker] WARN io.opentelemetry.javaagent.tooling.instrumentation.MuzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.logback.mdc.v1_0.LoggingEventInstrumentation$GetMdcAdvice:84 Missing class io.opentelemetry.javaagent.tooling.ConfiguredResourceAttributesHolder

Place that class in https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap

breedx-splk

Few comments below.

...p/src/main/java/io/opentelemetry/javaagent/bootstrap/ConfiguredResourceAttributesHolder.java

...c/test/java/io/opentelemetry/javaagent/bootstrap/ConfiguredResourceAttributesHolderTest.java

cleverchuk · 2023-10-12T14:50:09Z

...tion/log4j/log4j-context-data/log4j-context-data-2.17/library-autoconfigure/build.gradle.kts

@@ -5,6 +5,7 @@ plugins {
 base.archivesName.set("${base.archivesName.get()}-autoconfigure")

 dependencies {
+ implementation(project(":javaagent-bootstrap"))


🤔 Is this safe?

no, it is not. library modules can not depend on :javaagent-bootstrap

What is the appropriate way to provide the resource attribute for the mdc here?

I think it would be acceptable to have a compile only dependency and in the code first verify that the class is present with Class.forName before using it.

Like this?

try { Class.forName("io.opentelemetry.javaagent.bootstrap.ConfiguredResourceAttributesHolder"); contextData.putAll(ConfiguredResourceAttributesHolder.getResourceAttributes()); } catch (ClassNotFoundException ignore) { }

typically you'd do the class presence check only once in class initializer and set a boolean based on that and use that boolean everywhere else

@laurit how does it look?

laurit · 2023-10-24T09:20:35Z

.../opentelemetry/instrumentation/log4j/contextdata/v2_17/OpenTelemetryContextDataProvider.java

@@ -25,6 +26,19 @@
 * #supplyContextData()} is called when a log entry is created.
 */
 public class OpenTelemetryContextDataProvider implements ContextDataProvider {
+
+ private boolean configuredResourceAttributeAccessible;


change this to a static final field and change what you have in constructor into a static method that returns a boolean and use it to initialize the field

laurit · 2023-10-25T18:14:07Z

...p/src/main/java/io/opentelemetry/javaagent/bootstrap/ConfiguredResourceAttributesHolder.java

+
+ private static String[] getConfiguredAttributes() {
+ String resourceAttributes =
+ ConfigPropertiesUtil.getString("otel.instrumentation.mdc.resource-attributes");


I think it would be better to use InstrumentationConfig.get().getList("otel.instrumentation.mdc.resource-attributes", emptyList()) but InstrumentationConfig isn't probably accessible from here. InstrumentationConfig gets the properties from the sdk and could support more source than what ConfigPropertiesUtil uses. ConfigPropertiesUtil is usually used for library instrumentations and InstrumentationConfig in javaagent. Could you try moving this class to https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/bootstrap/internal so you could use InstrumentationConfig. Sorry for not noticing this before, should be the last thing from me.

laurit · 2023-10-31T11:53:12Z

@cleverchuk I create a PR for your PR cleverchuk#1 that adds tests and makes it so that the resource attributes are added to mdc even if there is no active span.

laurit · 2023-11-07T12:30:42Z

@cleverchuk could you merge cleverchuk#1

…ation.mdc.resource-attributes` that will be used to configure `Resource` attributes to be added to logging map diagnostic context.

- use assertJ for unit test assertions - use plural for method and field names - add compile only dependency on `:javaagent-bootstrap` for `instrumentation:log4j:log4j-context-data:log4j-context-data-2.17:library-autoconfigure`

- use implementation dependency on `:javaagent-bootstrap`

- use compile dependency on `:javaagent-bootstrap` and do runtime check before use.

- refactor constructor to static method

- move ConfiguredResourceAttributesHolder to javaagent-extension-api

trask · 2023-11-09T15:43:54Z

@cleverchuk can you add docs for this? e.g. in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/logback/logback-mdc-1.0/javaagent/README.md

laurit · 2023-11-13T11:18:15Z

@trask I added the docs, could you review

trask · 2023-11-13T15:47:04Z

thx @laurit

wdyt of renaming

otel.instrumentation.mdc.resource-attributes

to

otel.instrumentation.common.mdc.resource-attributes

similar to discussion at #9758 (comment)?

…tation.common.mdc.resource-attributes

laurit · 2023-11-13T17:00:47Z

@trask renamed

cleverchuk force-pushed the cc/add-config-based-mdc-attribute branch 3 times, most recently from cf86974 to 4e0102e Compare October 10, 2023 20:42

cleverchuk force-pushed the cc/add-config-based-mdc-attribute branch 2 times, most recently from 8d344b5 to 1c02310 Compare October 11, 2023 13:43

cleverchuk marked this pull request as ready for review October 11, 2023 14:34

cleverchuk requested a review from a team as a code owner October 11, 2023 14:34

breedx-splk reviewed Oct 11, 2023

View reviewed changes

cleverchuk force-pushed the cc/add-config-based-mdc-attribute branch from 33e527a to 07c9fca Compare October 12, 2023 12:33

cleverchuk commented Oct 12, 2023

View reviewed changes

cleverchuk requested a review from breedx-splk October 12, 2023 14:50

cleverchuk force-pushed the cc/add-config-based-mdc-attribute branch from 1f02dc6 to 9527b78 Compare October 16, 2023 18:07

cleverchuk requested a review from laurit October 17, 2023 12:26

cleverchuk force-pushed the cc/add-config-based-mdc-attribute branch 2 times, most recently from cd4020c to 6ec28c2 Compare October 17, 2023 17:35

laurit reviewed Oct 24, 2023

View reviewed changes

cleverchuk force-pushed the cc/add-config-based-mdc-attribute branch from 6ec28c2 to 8653b1c Compare October 25, 2023 14:34

cleverchuk requested a review from laurit October 25, 2023 14:34

laurit reviewed Oct 25, 2023

View reviewed changes

cleverchuk force-pushed the cc/add-config-based-mdc-attribute branch from 80bea6d to 857db6e Compare October 26, 2023 18:26

cleverchuk requested a review from laurit October 26, 2023 19:40

cleverchuk added 5 commits November 9, 2023 08:10

(open-telemetry#9297) add new instrumentation config `otel.instrument…

32e4a29

…ation.mdc.resource-attributes` that will be used to configure `Resource` attributes to be added to logging map diagnostic context.

(open-telemetry#9297)

a0bf776

- use assertJ for unit test assertions - use plural for method and field names - add compile only dependency on `:javaagent-bootstrap` for `instrumentation:log4j:log4j-context-data:log4j-context-data-2.17:library-autoconfigure`

(open-telemetry#9297)

f4fb878

- use implementation dependency on `:javaagent-bootstrap`

(open-telemetry#9297)

153b74b

- use compile dependency on `:javaagent-bootstrap` and do runtime check before use.

(open-telemetry#9297)

d5620f8

- refactor constructor to static method

cleverchuk and others added 3 commits November 9, 2023 08:10

(open-telemetry#9297)

9cf2689

- move ConfiguredResourceAttributesHolder to javaagent-extension-api

Add tests for resource attributes in mdc

740ef8a

fix test

4e80330

cleverchuk force-pushed the cc/add-config-based-mdc-attribute branch from 5fe7b75 to 4e80330 Compare November 9, 2023 13:13

laurit approved these changes Nov 9, 2023

View reviewed changes

add doc

22fde67

rename otel.instrumentation.mdc.resource-attributes to otel.instrumen…

822f0b1

…tation.common.mdc.resource-attributes

trask merged commit 81f8bf6 into open-telemetry:main Nov 13, 2023
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `service.name` to MDC #9647

Add `service.name` to MDC #9647

cleverchuk commented Oct 10, 2023

linux-foundation-easycla bot commented Oct 10, 2023 •

edited

Loading

laurit commented Oct 11, 2023

breedx-splk left a comment

cleverchuk Oct 12, 2023

laurit Oct 12, 2023

cleverchuk Oct 13, 2023

laurit Oct 13, 2023

cleverchuk Oct 16, 2023

laurit Oct 16, 2023

cleverchuk Oct 20, 2023 •

edited

Loading

laurit Oct 24, 2023

laurit Oct 25, 2023

laurit commented Oct 31, 2023

laurit commented Nov 7, 2023

trask commented Nov 9, 2023

laurit commented Nov 13, 2023

trask commented Nov 13, 2023

laurit commented Nov 13, 2023

Add service.name to MDC #9647

Add service.name to MDC #9647

Conversation

cleverchuk commented Oct 10, 2023

linux-foundation-easycla bot commented Oct 10, 2023 • edited Loading

laurit commented Oct 11, 2023

breedx-splk left a 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

cleverchuk Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurit commented Oct 31, 2023

laurit commented Nov 7, 2023

trask commented Nov 9, 2023

laurit commented Nov 13, 2023

trask commented Nov 13, 2023

laurit commented Nov 13, 2023

Add `service.name` to MDC #9647

Add `service.name` to MDC #9647

linux-foundation-easycla bot commented Oct 10, 2023 •

edited

Loading

cleverchuk Oct 20, 2023 •

edited

Loading