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

Replace print with log messages. #1447

Merged
merged 3 commits into from
Oct 23, 2020

Conversation

vadeg
Copy link
Contributor

@vadeg vadeg commented Oct 21, 2020

Replace print with log messages.

  • Add slf4j compile time dependency to the javaagent-exporters.logging module
  • Replace System.out.print with log.debug messages

Concerns:

  1. I am not sure about log.debug. I can replace it if you think that it should be in other level.
  2. AttributeKey ls logged using toString() method from its implementation. The only implementation right now is AutoValue_AttributeKeyImpl which prints the key like "AttributeKeyImpl{getType=" + this.getType + ", key=" + this.key + "}". I think it is not a good idea to use toString from objects for logging because anyone can change the method's implementation or provide implementation without toString (with default) and it will affect logging quality. I would change it not rely on toString output, but I don't know what would the appropriate format for this. If you have ideas please share.

Fixes: #1328

* Add `slf4j` compile time dependency to the `javaagent-exporters.logging` module
* Replace `System.out.println` with log.debug messages

Fixes: open-telemetry#1328
@vadeg vadeg force-pushed the gh-1328-replace-sout-with-log branch from be44e52 to 069397e Compare October 21, 2020 21:04
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks for the help!

Copy link
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

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

What Anuraag said

vadeg and others added 2 commits October 22, 2020 22:26
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
@vadeg
Copy link
Contributor Author

vadeg commented Oct 22, 2020

Updated with suggestions from @anuraaga

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.

👍

@iNikem iNikem merged commit 3cf2a35 into open-telemetry:master Oct 23, 2020
@vadeg vadeg deleted the gh-1328-replace-sout-with-log branch October 24, 2020 16:40
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.

Change logging exporter from System.out to SLF4J
4 participants