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

Should instrumentation-api (and library instrumentation) continue using slf4j? #5053

Closed
trask opened this issue Jan 9, 2022 · 9 comments
Closed

Comments

@trask
Copy link
Member

trask commented Jan 9, 2022

Currently instrumentation-api depends on slf4j, while OpenTelemetry API depends only on java.util.logging.

@iNikem
Copy link
Contributor

iNikem commented Jan 9, 2022

Does anybody remember what was the original reason not to use jul?

@anuraaga
Copy link
Contributor

anuraaga commented Jan 9, 2022

I suspect it's inherited from dd-trace.

We would probably want to be consistent and use jul everywhere, not just instrumentation API. Because it's in the JDK it can't really cause dependency conflicts I guess - would it allow us to remove patch logger and even bridge into the user's app if they use a jul-slf4j or similar bridge? That seems like it could be a nice UX improvement.

@laurit
Copy link
Contributor

laurit commented Jan 9, 2022

Using jul is problematic because applications, for example wildfly, may attempt to use a custom log manager by calling System.setProperty("java.util.logging.manager", logManagerName) from main. Unfortunately this works only when jul has not been initialized before that call. If we'd want to use jul in agent we'd need to hold of using logging until application has managed to set up logging to avoid breaking the application. Imo using jul in agent without breaking wildfly is going to be a challenge.

@trask
Copy link
Member Author

trask commented Jan 9, 2022

It sounds like there's agreement on using jul at least in the instrumentation-api and library instrumentation.

I agree with @laurit that using jul in the javaagent has significant issues that we'd have to weigh carefully. I'll create a separate issue to track bridging agent logging to users app logging since that has come up a couple of times now (#3413 (comment)).

@anuraaga
Copy link
Contributor

Is it possible to use jul in instrumentation API but slf4j in agent without creating a logging API to pick one or the other? Or can we assume usage of instrumentation API would be late enough to use jul even from agent?

Also recently I added a jul debug statement in the SDK when finishing autoconfiguration - do we expect that (will be released in 1.10) to break wildfly, if debug logging is enabled?

We have many cool tricks in the agent for corner cases, such as hiding methods from reflection. It would be cool if there is a trick by instrumenting jul to make using jul safe :)

@trask
Copy link
Member Author

trask commented Jan 10, 2022

Is it possible to use jul in instrumentation API but slf4j in agent without creating a logging API to pick one or the other? Or can we assume usage of instrumentation API would be late enough to use jul even from agent?

Also recently I added a jul debug statement in the SDK when finishing autoconfiguration - do we expect that (will be released in 1.10) to break wildfly, if debug logging is enabled?

this is what PatchLogger is for, we shade all of the agent's (transitive) usages of jul so that they really use PatchLogger -> slf4j

@anuraaga
Copy link
Contributor

Ah oops that makes sense. Does it mean we would be able to use jul in agent code too, though it doesn't magically allow bridging into the app? It would be nice to avoid using two different logging APIs in the same codebase.

@trask
Copy link
Member Author

trask commented Jan 10, 2022

Does it mean we would be able to use jul in agent code too, though it doesn't magically allow bridging into the app? It would be nice to avoid using two different logging APIs in the same codebase.

yeah, this should work

@trask
Copy link
Member Author

trask commented Jan 11, 2022

Discussed in today's SIG meeting, and decided:

  • we should use jul in the instrumentation-api and library instrumentation to be consistent with SDK and limit dependencies
  • we should use jul in javaagent instrumentation because using two different logging libraries feels icky

For now at least, the javaagent will continue to shade all jul usage to PatchLogger.java for the reasons @laurit mentioned above #5053 (comment), and we will explore redirecting logging to user application logging separately in #5059

Closing this discussion issue and will open a new issue with this plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants