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

Encapsulate logging #6543

Merged
merged 5 commits into from
Sep 12, 2022
Merged

Conversation

mateuszrzeszutek
Copy link
Member

Depends on #6499

This PR introduces a new javaagent-internal-logging-simple module that encapsulates the logging implementation completely (well, almost, except for a couple of tests). Slf4j is removed from bootstrap dependencies, shading relocation lists, and from the agent's "public" (as in, publicly accessible) space.

@mateuszrzeszutek mateuszrzeszutek marked this pull request as ready for review September 8, 2022 10:07
@mateuszrzeszutek mateuszrzeszutek requested a review from a team September 8, 2022 10:07
Comment on lines -17 to -18
// Prevents conflict with other SLF4J instances. Important for premain.
relocate("org.slf4j", "io.opentelemetry.javaagent.slf4j")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -92,12 +92,12 @@ class JavaagentTestArgumentsProvider(
"-Dotel.metrics.exporter=otlp",
// suppress repeated logging of "No metric data to export - skipping export."
// since PeriodicMetricReader is configured with a short interval
"-Dio.opentelemetry.javaagent.slf4j.simpleLogger.log.io.opentelemetry.sdk.metrics.export.PeriodicMetricReader=INFO",
"-Dio.opentelemetry.javaagent.logging.simple.slf4j.simpleLogger.log.io.opentelemetry.sdk.metrics.export.PeriodicMetricReader=INFO",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow thats quite a property name 😂


// org.slf4j package name in the constants will be shaded too
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +42 to +43
// trigger loading the provider from the agent CL
LoggerFactory.getILoggerFactory();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need to trigger loading?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably - the first usage of any slf4j API method causes it to load the actual implementation using SPI; and since it uses the context CL, it might not find the actual implementation. It's safer to just pre-load it here.

javaagent-internal-logging-simple/build.gradle.kts Outdated Show resolved Hide resolved
Mateusz Rzeszutek and others added 2 commits September 9, 2022 13:12
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
mergeServiceFiles()

// Prevents configuration naming conflict with other SLF4J instances
relocate("org.slf4j", "io.opentelemetry.javaagent.logging.simple.slf4j")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be better to continue using io.opentelemetry.javaagent.slf4j as there are people who are using stuff like -Dio.opentelemetry.javaagent.slf4j.simpleLogger.defaultLogLevel=off and -Dio.opentelemetry.javaagent.slf4j.simpleLogger.logFile=...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, you're right. I dislike the fact that we're exposing the javaagent internals this way, but since we don't have a better logging story I suppose it is a necessity for now. I'll revert to the previous package name.

@trask trask merged commit 8b2b328 into open-telemetry:main Sep 12, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the encapsulate-logging branch September 13, 2022 09:13
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
* Encapsulate actual logging implementation better

* Apply suggestions from code review

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

* code review comments

* revert to the old slf4j package name

* spotless

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
* Encapsulate actual logging implementation better

* Apply suggestions from code review

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

* code review comments

* revert to the old slf4j package name

* spotless

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
* Encapsulate actual logging implementation better

* Apply suggestions from code review

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

* code review comments

* revert to the old slf4j package name

* spotless

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
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.

3 participants