Bridge agent logs into application's slf4j logger #7339

mateuszrzeszutek · 2022-11-29T16:45:51Z

Related discussion #7257
Resolves #3413
Resolves #5059
Resolves #6258
Resolves #7179

Adds a logging implementation that'll collect agent logs in memory until slf4j is detected in the instrumented application; and when that happens will dump all the logs into the application slf4j and log directly to the application logger from that time on.

It's still in a POC state, unfortunately: while it works fine with an app that uses & initializes slf4j directly, Spring Boot applications actually reconfigure the logging implementation (e.g. logback) a while after slf4j is loaded; which causes all the startup agent logs (debug included) to be dumped with the default logback pattern.

Future work:

~~Make sure all logs produces by the agent are sent to loggers named io.opentelemetry... (Muzzle logs should be logged using the io.opentelemetry.* logger name #7446)~~ DONE
Make this work on Spring Boot
Documentation
Smoke test?

breedx-splk

This seems cool to me. I'd love a small demo to see it in action, maybe in the sig somtime?

...on-api/src/main/java/io/opentelemetry/instrumentation/api/internal/ConfigPropertiesUtil.java

breedx-splk · 2022-12-02T21:50:01Z

.../io/opentelemetry/javaagent/instrumentation/internal/slf4j/LoggerFactoryInstrumentation.java

+
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void onExit() {
+      Slf4jApplicationLoggerBridge.install();


It's not immediately clear to me why this installation needs to happen in this instrumentation. Is there not another lifecycle hook that makes sense for this? If not (like maybe we only want to do this when slf4j is actually in the runtime mix?), maybe a comment could help to clarify why this should be wired via bytecode instead of agent lifecycle hookery.

Is there not another lifecycle hook that makes sense for this?

There is not - this needs to be initialized from the application code, because we need to grab a handle to the application's slf4j. This can pretty much only be done in an instrumentation.

breedx-splk · 2022-12-02T22:03:29Z

...va/io/opentelemetry/javaagent/instrumentation/internal/slf4j/Slf4jInstrumentationModule.java

+  @Override
+  public boolean defaultEnabled(ConfigProperties config) {
+    // only enable this instrumentation if the application logger is enabled
+    return config.getString("otel.javaagent.logging", "simple").equalsIgnoreCase("application");


It looks like this was around before this PR, but I don't see mention of it in any readme/notes/docs (and I also search the docs repo). Can we please get this added somewhere or an issue created to do that work?

Yeah, I was planning to add some docs once this PR is accepted. Will create an issue when this is merged.

...nt-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/ApplicationLoggerBridge.java

breedx-splk · 2022-12-02T22:12:53Z

...nt-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/ApplicationLoggerBridge.java

+    if (bridge != null) {
+      bridge.install(applicationLoggerFactory);
+    }
+  }


The relationship between set() and installApplicationLogger() is a little confusing. I wish there was just one method...but I'm sure there are timing considerations that make this challenging.

Yep, there need to be 2 steps to initialize this thing. The names could be better, but I didn't manage to think up anything less confusing. Suggestions welcome 😄

...plication/src/main/java/io/opentelemetry/javaagent/logging/application/InMemoryLogStore.java

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/NoopLoggingCustomizer.java

...ng-application/src/main/java/io/opentelemetry/javaagent/logging/application/InMemoryLog.java

…#7446) Also, log them on DEBUG when debug mode is off. Related to #7339

Fixes #7540 I think this kind of early config might be useful in general -- for instance, I think I'd like to use it for properties introduced in #7339

mateuszrzeszutek · 2023-03-30T15:27:23Z

Rebased with main, @open-telemetry/java-instrumentation-maintainers PTAL

...trap/src/main/java/io/opentelemetry/javaagent/bootstrap/logging/ApplicationLoggerBridge.java

...etry/javaagent/instrumentation/internal/logging/ApplicationLoggingInstrumentationModule.java

...o/opentelemetry/javaagent/instrumentation/internal/logging/LoggerFactoryInstrumentation.java

...n/src/main/java/io/opentelemetry/javaagent/logging/application/ApplicationLoggerFactory.java

...c/main/java/io/opentelemetry/javaagent/logging/application/ApplicationLoggingCustomizer.java

...plication/src/main/java/io/opentelemetry/javaagent/logging/application/InMemoryLogStore.java

...etry/javaagent/instrumentation/internal/logging/ApplicationLoggingInstrumentationModule.java

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java

mateuszrzeszutek requested a review from a team as a code owner November 29, 2022 16:45

breedx-splk reviewed Dec 2, 2022

View reviewed changes

mateuszrzeszutek force-pushed the application-logging branch 2 times, most recently from e235600 to 0fcbedc Compare December 12, 2022 09:32

mateuszrzeszutek mentioned this pull request Dec 12, 2022

Logging agent logs separately #7179

Closed

mateuszrzeszutek force-pushed the application-logging branch from 0fcbedc to 4ad2805 Compare December 16, 2022 09:53

mateuszrzeszutek mentioned this pull request Dec 19, 2022

Muzzle logs should be logged using the io.opentelemetry.* logger name #7446

Merged

mateuszrzeszutek mentioned this pull request Jan 11, 2023

Make config file available in early agent initialization phase #7550

Merged

trask pushed a commit that referenced this pull request Jan 12, 2023

Muzzle logs should be logged using the io.opentelemetry.* logger name (…

bb7a12d

…#7446) Also, log them on DEBUG when debug mode is off. Related to #7339

Mateusz Rzeszutek added 7 commits March 30, 2023 15:38

Bridge agent logs into application's slf4j logger

4186651

code review comments

3ad196a

move a method

6430295

add missing test

0e08169

Make InternalLogger an interface

3ea87ec

Use explicit classloader in ServiceLoader.load()

c0b24bd

Rename internal instrumentation module

3cf626f

mateuszrzeszutek force-pushed the application-logging branch from 4ad2805 to 3cf626f Compare March 30, 2023 15:26

spotless

3d4d5f3

trask approved these changes Apr 3, 2023

View reviewed changes

Mateusz Rzeszutek added 2 commits April 3, 2023 11:37

code review comments

f0fdd7c

checkstyle

a495b98

trask approved these changes Apr 3, 2023

View reviewed changes

mateuszrzeszutek merged commit 04f2e3e into open-telemetry:main Apr 4, 2023

mateuszrzeszutek deleted the application-logging branch April 4, 2023 15:29

This was referenced Apr 4, 2023

[Application logger bridge] Implement Spring Boot support #8210

Closed

[Application logger bridge] Document the feature #8211

Closed

[Application logger bridge] Implement tests #8212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bridge agent logs into application's slf4j logger #7339

Bridge agent logs into application's slf4j logger #7339

mateuszrzeszutek commented Nov 29, 2022 •

edited

Loading

breedx-splk left a comment

breedx-splk Dec 2, 2022

mateuszrzeszutek Dec 8, 2022

breedx-splk Dec 2, 2022

mateuszrzeszutek Dec 8, 2022

breedx-splk Dec 2, 2022

mateuszrzeszutek Dec 8, 2022

mateuszrzeszutek commented Mar 30, 2023

Bridge agent logs into application's slf4j logger #7339

Bridge agent logs into application's slf4j logger #7339

Conversation

mateuszrzeszutek commented Nov 29, 2022 • edited Loading

breedx-splk left a comment

Choose a reason for hiding this comment

breedx-splk Dec 2, 2022

Choose a reason for hiding this comment

mateuszrzeszutek Dec 8, 2022

Choose a reason for hiding this comment

breedx-splk Dec 2, 2022

Choose a reason for hiding this comment

mateuszrzeszutek Dec 8, 2022

Choose a reason for hiding this comment

breedx-splk Dec 2, 2022

Choose a reason for hiding this comment

mateuszrzeszutek Dec 8, 2022

Choose a reason for hiding this comment

mateuszrzeszutek commented Mar 30, 2023

mateuszrzeszutek commented Nov 29, 2022 •

edited

Loading