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 autoconfigure console alias for logging exporter #6027

Merged

Conversation

jack-berg
Copy link
Member

The spec recently merged a series of PRs which officially defines the "stdout" exporters, and defines the corresponding OTEL_{SIGNAL}_EXPORTERS value to be console:

This PR reflects those changes by adding support for the console value, which is simply an alias for logging.

@jack-berg jack-berg requested a review from a team as a code owner November 29, 2023 15:30
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: Patch coverage is 96.77419% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.05%. Comparing base (62a4810) to head (62c57f9).

Files Patch % Lines
...ogging/internal/ConsoleMetricExporterProvider.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6027      +/-   ##
============================================
- Coverage     91.06%   91.05%   -0.01%     
- Complexity     5699     5711      +12     
============================================
  Files           621      624       +3     
  Lines         16679    16699      +20     
  Branches       1709     1711       +2     
============================================
+ Hits          15188    15206      +18     
- Misses          998      999       +1     
- Partials        493      494       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -27,6 +27,7 @@ final class LogRecordExporterConfiguration {

static {
EXPORTER_ARTIFACT_ID_BY_NAME = new HashMap<>();
EXPORTER_ARTIFACT_ID_BY_NAME.put("console", "opentelemetry-exporter-logging");
Copy link
Member

Choose a reason for hiding this comment

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

what do you think of renaming the artifact?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be a little hesitant in renaming artifacts. It's definitely caused a lot of confusion in the past when suddenly an artifact no longer exists for a version, and it's been renamed. This has often led to people mixing and matching artifact versions, which is a significant foot gun for users.

Copy link
Member Author

Choose a reason for hiding this comment

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

An argument against renaming artifacts is that it will cause the class names to be misaligned:

  • Artifact named opentelemetry-exporter-console
  • Classes named Logging{Signal}Exporter

Of course we could introduce new classes named Console{Signal}Exporter and deprecate the Logging{Signal}Exporter variations.

Copy link
Member

Choose a reason for hiding this comment

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

I find the name opentelemetry-exporter-logging to be regularly confusing (I keep thinking it's the log exporter).

I'd be a little hesitant in renaming artifacts. It's definitely caused a lot of confusion in the past when suddenly an artifact no longer exists for a version, and it's been renamed. This has often led to people mixing and matching artifact versions, which is a significant foot gun for users.

this is a great point though

maybe we can have both opentelemetry-exporter-console and keep publishing (a deprecated) opentelemetry-exporter-logging artifact (which can have a dependency on opentelemetry-exporter-console if that's helpful for reducing duplication)?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we can have both opentelemetry-exporter-console and keep publishing (a deprecated) opentelemetry-exporter-logging artifact (which can have a dependency on opentelemetry-exporter-console if that's helpful for reducing duplication)?

That's an interesting idea..

While we're on the subject of making the logging exporters less confusing / more useful, would probably be good to optionally give the user more control over where the logs go. Could have an optional Consumer<String> logHandler argument which defaults to sending to JUL, but could also be configured to go to the user's desired logger (SLF4J, System.out, etc). Could apply the same principle to the logging OTLP exporter.

Copy link
Member Author

@jack-berg jack-berg Feb 29, 2024

Choose a reason for hiding this comment

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

Opened #6263 to discuss further and track. Shouldn't be a blocker for this PR tho. Please take another look!

@jack-berg jack-berg added this to the 1.33.0 milestone Nov 30, 2023
@jack-berg jack-berg removed this from the 1.33.0 milestone Jan 2, 2024
@jack-berg jack-berg merged commit 7655192 into open-telemetry:main Mar 12, 2024
18 checks passed
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.

None yet

3 participants