-
Notifications
You must be signed in to change notification settings - Fork 851
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
Add autoconfigure console alias for logging exporter #6027
Conversation
Codecov ReportAttention: Patch coverage is
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. |
@@ -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"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
The spec recently merged a series of PRs which officially defines the "stdout" exporters, and defines the corresponding
OTEL_{SIGNAL}_EXPORTERS
value to beconsole
:This PR reflects those changes by adding support for the
console
value, which is simply an alias forlogging
.