Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to instrument logs before OTel injection into OTel appenders #9798

Merged
merged 56 commits into from
Nov 17, 2023

Conversation

jeanbisutti
Copy link
Member

The logs created before the injection of the OpenTelemetry instance into the OpenTelemetry appenders were not instrumented.

@jeanbisutti jeanbisutti requested a review from a team as a code owner November 2, 2023 14:34
@jeanbisutti
Copy link
Member Author

For Log4j, LogEvent has String getThreadName() and long getThreadId() methods. Maybe they coud be used in the LogEventMapper:

In this way, for cached logs, we could be sure to have the good thread name and thread values, for example in the case of an OTel injection not done from the main thread.

@jeanbisutti
Copy link
Member Author

A Logback ILoggingEvent object, has only a String getThreadName() method, not a long getThreadId method. Perhaps the current implementation to get the thread name and thread id could be kept:

In addition we could document for Logback that the cached logs could have a wrong thread name and thread id?

@trask
Copy link
Member

trask commented Nov 2, 2023

In addition we could document for Logback that the cached logs could have a wrong thread name and thread id?

I think I'd prefer the extra code to implement correct behavior and not need to remember about (or document) this edge case not working.

@jeanbisutti
Copy link
Member Author

Thread id and thread name are fixed for the replayed logs

@jeanbisutti
Copy link
Member Author

it seems like we pretty much need to run the full set of tests using both pre-install path and post-install path (since it would be easy to miss one of the pieces of opt-in data, and easy in the future to add a new opt-in data and forget to handle the pre-install path)

can we make this a base test class and have two subclasses, one which does install before the test, and one that delays the install?

tests refactored

jeanbisutti and others added 4 commits November 16, 2023 16:57
…a/io/opentelemetry/instrumentation/log4j/appender/v2_17/LogEventToReplay.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@trask trask added this to the v1.32.0 milestone Nov 16, 2023
@trask trask merged commit c5cb948 into open-telemetry:main Nov 17, 2023
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants