Support output_stream in declarative config of otlp_file/development - #8676
Support output_stream in declarative config of otlp_file/development#8676robintra wants to merge 2 commits into
Conversation
|
|
b3ab6ed to
3d94c71
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8676 +/- ##
============================================
+ Coverage 91.65% 91.66% +0.01%
- Complexity 10352 10358 +6
============================================
Files 1003 1004 +1
Lines 27210 27238 +28
Branches 3199 3203 +4
============================================
+ Hits 24939 24968 +29
+ Misses 1566 1564 -2
- Partials 705 706 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Pull request dashboard statusWaiting on the author · refreshed 2026-08-24 21:11 UTC Resolve merge conflicts. Respond to 5 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
054525a to
6d92d47
Compare
| * Logging: Support the `output_stream` option when declaratively configuring `otlp_file/development` | ||
| ([#8676](https://github.com/open-telemetry/opentelemetry-java/pull/8676)) |
There was a problem hiding this comment.
Don't both adding a changelog entry. They're added as part of the release process. This is in the wrong section anyway.
| if (outputStream == null) { | ||
| return; | ||
| } | ||
| if (STDOUT.equalsIgnoreCase(outputStream)) { |
There was a problem hiding this comment.
No need for ignoring case. Better to be more explicit and case sensitive. Can always expand if needed.
| } catch (URISyntaxException e) { | ||
| throw new ConfigurationException("Unrecognized output_stream: " + outputStream, e); | ||
| } | ||
| if (!FILE_SCHEME.equalsIgnoreCase(uri.getScheme())) { |
There was a problem hiding this comment.
Same here: match the scheme case-sensitively against "file". The spec's example is lowercase and a well-formed URI's scheme is already lowercase, so equalsIgnoreCase only accepts ill-formed input. Can loosen later if needed.
| static Stream<Arguments> outputStreamFileTestCases() { | ||
| return Stream.of( | ||
| Arguments.argumentSet("file uri", "test.jsonl", (UnaryOperator<String>) uri -> uri), | ||
| Arguments.argumentSet( | ||
| "authority-less file uri with upper case scheme", | ||
| "test.jsonl", | ||
| (UnaryOperator<String>) uri -> uri.replaceFirst("^file://", "FILE:")), | ||
| Arguments.argumentSet( | ||
| "missing parent directory", "missing/test.jsonl", (UnaryOperator<String>) uri -> uri)); | ||
| } |
There was a problem hiding this comment.
Once matching is strict (see the OutputStreamConfigUtil threads), the only case here that actually mutates the URI ("authority-less file uri with upper case scheme") goes away. The remaining two both pass identity, so the UnaryOperator<String> parameter collapses to just String relativePath and the test body can use file.toUri().toString() directly. Worth simplifying at the same time.
| throw new ConfigurationException("Unrecognized output_stream: " + outputStream, e); | ||
| } | ||
| if (!FILE_SCHEME.equalsIgnoreCase(uri.getScheme())) { | ||
| throw new ConfigurationException("Unrecognized output_stream: " + outputStream); |
There was a problem hiding this comment.
The "Unrecognized output_stream: <value>" message doesn't hint at what is expected. Consider "Unrecognized output_stream: <value>. Expected \"stdout\" or a file:// URI." so users get a pointer without having to consult the spec. Applies to all three ConfigurationException throw sites in filePath and the scheme-check.
Fixes #8675.
The OTLP file exporter builders already accept an arbitrary
OutputStreamand the generated config model already carriesoutput_stream, but the declarative configuration component providers never read it so only stdout was reachable without writing code. The support table inopentelemetry-configurationreports Java asnot_implementedfor this option, while C++ and PHP reportsupported.This wires the option through for spans, metrics and logs:
stdoutsetsSystem.outexplicitly, rather than relying on the default installed by the staticbuilder()factory. Matching is case-insensitive, like the sibling options parsed in the samecreate()call.java.net.URI, so bothfile:///pathand the authority-lessfile:/pathare accepted, with any scheme casing. Missing parent directories are created, the file is opened withCREATE+APPENDand the stream is wrapped in aBufferedOutputStream.StreamJsonWriterflushes after every record so buffering does not delay output.ConfigurationException, distinguishing an unusable value from a file that could not be opened.Two choices worth confirming:
Tests are added to
AbstractOtlpStdoutExporterTestso they run for all three signals:stdout, a plainfile://URI producing the expected JSON, the authority-less and upper case URI form, parent directory creation, an unrecognized value, and a path that cannot be opened.DeclarativeConfigurationCreateTest.parseAndCreate_Examplesneeded one adjustment. TheExperimentalOtlpFile*_file.yamlsnippets write tofile:///var/log/*.jsonl, which is not writable in tests. The test already rewritesca_file,key_fileandcert_fileto temp files for the same reason sooutput_streamis rewritten the same way.Two limitations are left as-is, since addressing either would go beyond wiring up the option. I am happy to follow up if maintainers want them handled here:
StreamJsonWriterdoes not synchronize so concurrent writes can interleave.Verification
./gradlew :exporters:logging-otlp:checkand./gradlew :sdk-extensions:declarative-config:checkpass.jApiCmpreports no API change since the new class lives in aninternalpackage sodocs/apidiffsis unchanged.Also verified end to end with a throwaway test that runs
DeclarativeConfiguration.parseAndCreateon:and confirms the span is written as one JSON line to the file with nothing on stdout.