OTLP Stdout exporter

[zeitlinger]

prototype implementation for open-telemetry/opentelemetry-specification#4183

Split into several PRs

• add stdout log record exporter #6675
• Stdout exporter for span and metrics #6750
• add stdout memory mode #6774

[zeitlinger]

@jack-berg can you take a look?

[jack-berg]

Why build all this over again instead of extending OtlpJsonLoggingSpanExporter with additional configuration options?

[zeitlinger]

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.

[jack-berg]

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();

[zeitlinger]

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

[zeitlinger]

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

[jack-berg]

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();

[jack-berg]

Is the memory mode really applicable?

Memory mode is pretty great. I think we can reuse the existing OtlpJsonLogging{Signal}Exporters for this, and we have #6474 to track adding memory mode support to those.

[zeitlinger]

Memory mode is pretty great.

that was also my assumption which I used for the pr 😄

[codecov]

Codecov Report

Attention: Patch coverage is 91.46341% with 42 lines in your changes missing coverage. Please review.

Project coverage is 90.64%. Comparing base (ad120a5) to head (8d8f14f).
Report is 65 commits behind head on main.

Files with missing lines	Patch %	Lines
...lemetry/exporter/internal/ExporterBuilderUtil.java	69.76%	10 Missing and 3 partials ⚠️
...logging/otlp/internal/writer/StreamJsonWriter.java	56.52%	9 Missing and 1 partial ⚠️
...logging/otlp/internal/writer/LoggerJsonWriter.java	66.66%	6 Missing ⚠️
...er/logging/otlp/OtlpJsonLoggingMetricExporter.java	92.85%	3 Missing and 1 partial ⚠️
...logging/otlp/OtlpJsonLoggingLogRecordExporter.java	94.11%	1 Missing and 1 partial ⚠️
...rter/logging/otlp/OtlpJsonLoggingSpanExporter.java	94.11%	1 Missing and 1 partial ⚠️
...try/exporter/logging/otlp/internal/AccessUtil.java	60.00%	2 Missing ⚠️
...r/internal/otlp/logs/LogReusableDataMarshaler.java	94.44%	0 Missing and 1 partial ⚠️
...rnal/otlp/metrics/MetricReusableDataMarshaler.java	94.44%	0 Missing and 1 partial ⚠️
...nternal/otlp/traces/SpanReusableDataMarshaler.java	94.44%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6632      +/-   ##
============================================
+ Coverage     90.05%   90.64%   +0.58%     
- Complexity     6275     6422     +147     
============================================
  Files           697      721      +24     
  Lines         18944    19228     +284     
  Branches       1858     1856       -2     
============================================
+ Hits          17060    17429     +369     
+ Misses         1307     1223      -84     
+ Partials        577      576       -1     

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

[zeitlinger]

@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?

[zeitlinger]

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

[jack-berg]

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:

• Add the SPI implementations in followups (env var and declarative config can each be separate)
• Demonstrate the concept with the exporter for one signal, then extend to the other signals
• Decouple adding support for memory mode

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.

[zeitlinger]

Need to find a way to make PRs more reviewable.

fair point - I've created in initial PR #6675 that has

• memory mode removed
• is only for log records

The component providers are very small, so I left them in.