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

Add documentation about Java Logger for Error Handling #1501

Conversation

MitchellDumovic
Copy link
Contributor

This PR follows from discussion on #1419.

This is intended to be a an "implementation" of open-telemetry/opentelemetry-specification#696

In the future, authors should continue to use java.util.logging with an appropriate log level to log messages, so that users have full control over the logging output by OpenTelemetry.

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #1501 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1501   +/-   ##
=========================================
  Coverage     87.03%   87.03%           
  Complexity     1369     1369           
=========================================
  Files           163      163           
  Lines          5205     5205           
  Branches        490      490           
=========================================
  Hits           4530     4530           
  Misses          493      493           
  Partials        182      182           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4680c3a...2392a68. Read the comment docs.

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!

QUICKSTART.md Outdated

## Logging and Error Handling

OpenTelemetry uses [java.util.logging](https://docs.oracle.com/javase/8/docs/api/java/util/logging/package-summary.html)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how much the API changes from 7 to 8, so this suggestion might not make sense.

Suggested change
OpenTelemetry uses [java.util.logging](https://docs.oracle.com/javase/8/docs/api/java/util/logging/package-summary.html)
OpenTelemetry uses [java.util.logging](https://docs.oracle.com/javase/7/docs/api/java/util/logging/package-summary.html)

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the logging we do (all?) happens in the SDK level, where we use Java 8+ ;)

Copy link
Member

Choose a reason for hiding this comment

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

The SDK also uses Java 7 AFAIK. If not I would be in trouble. 😄

Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/open-telemetry/opentelemetry-java/blob/v0.7.0/build.gradle#L37-L38. We only use Java 8 for (all) unit tests. But users might be able to use the full Java 8 (or 11 or newer) logging API for configuration, even though the SDK uses only the Java 7 subset for producing the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell the API does not change significantly from 7 to 8 - all the examples here should still be relevant. Though I agree that it makes the most sense to link the most accurate docs here, I will change this.

@jkwatson jkwatson merged commit c442300 into open-telemetry:master Aug 4, 2020
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.

6 participants