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

Enable otlp logs by default #5433

Merged

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented May 8, 2023

Related to #5340.

@jack-berg jack-berg mentioned this pull request May 8, 2023
7 tasks
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (47ee573) 91.30% compared to head (63dcadc) 91.31%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5433   +/-   ##
=========================================
  Coverage     91.30%   91.31%           
- Complexity     4887     4889    +2     
=========================================
  Files           548      548           
  Lines         14404    14409    +5     
  Branches       1353     1354    +1     
=========================================
+ Hits          13152    13158    +6     
+ Misses          867      866    -1     
  Partials        385      385           
Impacted Files Coverage Δ
.../autoconfigure/LogRecordExporterConfiguration.java 100.00% <100.00%> (+6.06%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jack-berg jack-berg force-pushed the enable-otlp-logs-by-default branch from 570413a to 63dcadc Compare May 10, 2023 14:19
@jack-berg jack-berg marked this pull request as ready for review May 10, 2023 14:19
@jack-berg jack-berg requested a review from a team May 10, 2023 14:19
@jkwatson
Copy link
Contributor

This is an interesting one. My guess is, that most people already have existing logging solutions that are already working, that they won't want to have to manually disable OTLP logs on their apps. This will only be a good experience for people who have a collector/backend that has log ingest enabled. We should be ready to revert this, based on user feedback.

@jack-berg
Copy link
Member Author

I think this should be enabled by default because:

  • The spec says we should and that section of the spec is stable.
  • Enabling otlp by default just means that log records passed through the SdkLoggerProvider are exported by default. A user of autoconfigure still has to wire SdkLoggerProvider into their log appender.
  • Its users of the agent that may already have log solutions and therefore not want OTLP logs by default. The agent is one of the main consumers of autoconfigure so maybe its typical use cases should influence the defaults of autoconfigure? I don't think so. If we find that most agent users don't want otel logs, we could change its behavior to not install the log appenders by default. So keep otlp as the default for SdkLoggerProvider, but stop application logs from flowing into SdkLoggerProvider.
  • Events also flow through SdkLoggerProvider.

@Izzzu
Copy link

Izzzu commented May 16, 2023

Hi!
I just came across this PR looking for an explanation why OTEL_LOGS_EXPORTER is not by default "otlp" as mentioned on this documentation page. It appears as one of the first in the search. Since this documentation does not mention any specific sdk it seems to be a contract description for any sdk, including java.

So from the user experience I think it would be helpful to have the default logs exporter set to "otlp" as it's consistent with the metrics and traces exporters and aligned across the docs.

@jack-berg jack-berg merged commit 85f3fd8 into open-telemetry:main May 17, 2023
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.

4 participants