OTLP Stdout exporter #6632
OTLP Stdout exporter #6632
Conversation
|
@jack-berg can you take a look? |
| import java.util.Collection; | ||
| import java.util.Deque; | ||
|
|
||
| public class OtlpSpanExporter { |
There was a problem hiding this comment.
Why build all this over again instead of extending OtlpJsonLoggingSpanExporter with additional configuration options?
There was a problem hiding this comment.
I tried that first - it turned out that the code reuse is much easier to do with the otlp exporters, which have the same options, such as memory mode and metrics aggregation things.
I didn't include the full code reuse, because that would make the pr bigger.
There was a problem hiding this comment.
What is different about this new exporter compared to OtlpJsonLoggingSpanExporter? Why can't we add a builder pattern to OtlpJsonLoggingSpanExporter to make the output stream configurable?
OtlpJsonLoggingSpanExporter.builder().setOutputStream(os).build();
There was a problem hiding this comment.
great point: the produced output is different.
- logging-json starts with
{ "record"- https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/logging-otlp/src/test/java/io/opentelemetry/exporter/logging/otlp/OtlpJsonLoggingLogRecordExporterTest.java#L100 - otlp-stdout starts with
{"resourceLogs":- https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/file-exporter.md#examples
There was a problem hiding this comment.
great point: the produced output is different.
- logging-json starts with
{ "record"- https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/logging-otlp/src/test/java/io/opentelemetry/exporter/logging/otlp/OtlpJsonLoggingLogRecordExporterTest.java#L100 - otlp-stdout starts with
{"resourceLogs":- https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/file-exporter.md#examples
There was a problem hiding this comment.
That seems like another config option:
OtlpJsonLoggingSpanExporter.builder()
.setOutputStream(os)
// Wrap Resource{Signal} with Export{Signal}ServiceRequest message
.wrapWithExportRequestMessage() // ..or some other better method name
.build();
Memory mode is pretty great. I think we can reuse the existing |
that was also my assumption which I used for the pr 😄 |
zeitlinger
force-pushed
the
stdout-exporter
branch
from
7b29293
to
069ce26
Compare
|
@jack-berg I started completely new after our discussion (thanks for the feedback). I still haven't tested the component provider. I think I need a DefaultStructuredConfigProperties to do that. Is this the right way? |
it was easy to do with mockito |
|
Need to find a way to make PRs more reviewable. Sometimes its unavoidable, but in this case, there are several of things that can be broken up, which would considerably shrink the PR:
With this big of a PR, I'm forced to either: 1. spend a couple of hours for each round of a detailed review. 2. give a cursory review and potentially miss bugs, or conceptual problems (not willing to do this) With this being just a prototype for something that does not yet exist in the spec, I'm afraid I just can't justify the time required to do a detailed review. Reminder that I'm the only full time maintainer of this repo and my current focus for the project is delivering declarative configuration. Apologies for not making this recommendation with the first iteration of this PR - that's on me. 
|
fair point - I've created in initial PR #6675 that has
The component providers are very small, so I left them in. |
prototype implementation for open-telemetry/opentelemetry-specification#4183
Split into several PRs