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

Muzzle mismatch logs should be warnings in tests #1754

Conversation

mateuszrzeszutek
Copy link
Member

Resolves #534

This change actually changes muzzle mismatch logs to warnings everywhere, not only in tests - but hides them behind the otel.javaagent.debug config.

@iNikem
Copy link
Contributor

iNikem commented Nov 25, 2020

If that actually works as intended, then 👍

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

I don't totally love altering the log level based on the debug flag. If I see something logged at warn when the debug flag is on, I think I'd be confused not to see that also logged when the debug flag is off, and info+ is (generally) being logged.

But I don't hate it either 😄, so good with it until (if) we think of something better.

`-Dio.opentelemetry.javaagent.slf4j.simpleLogger.defaultLogLevel=debug`
`-Dotel.javaagent.debug=true`
Copy link
Member

Choose a reason for hiding this comment

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

👍

@iNikem
Copy link
Contributor

iNikem commented Nov 26, 2020

But it is our original desire: to have muzzle logs on WARN, but do not log them by default. Which means that some WARN logs will be absent anyway when enabling only info or debug as default logging level.

@mateuszrzeszutek
Copy link
Member Author

I don't totally love altering the log level based on the debug flag. If I see something logged at warn when the debug flag is on, I think I'd be confused not to see that also logged when the debug flag is off, and info+ is (generally) being logged.

Hmm, maybe it's just a matter of naming the debug flag more clearly: if we used something like otel.javaagent.log-everything instead of otel.javaagent.debug the intent would be more obvious (don't treat the log-everything name seriously, that's just an idea 😅 )

@trask
Copy link
Member

trask commented Nov 26, 2020

I think ideally we would do something like #566 (comment)

log at a higher level than DEBUG if all instrumentation for a particular library (e.g. both cassandra-3.0 and cassandra-4.0) are muzzled

also, our debug logging is very verbose, which makes things get lost easily, even info/warnings

we could consider not using the debug flag during tests (by default), especially after #1643, when we can have separate log level for testing code (e.g. nice to log full spans that are received from exporter)

just stuff to think about / come back to later

i'm good with this PR as-is, it does address a real need 👍

@trask trask merged commit 90ecff5 into open-telemetry:master Nov 26, 2020
@mateuszrzeszutek mateuszrzeszutek deleted the muzzle-mismatch-logs-should-be-warnings-in-tests branch February 5, 2021 11:39
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.

Muzzle bytecode runtime logs could be warnings, at least for tests
3 participants