add stdout log record exporter

[zeitlinger]

split off from #6632 - only for logs, and no memory mode

[codecov]

Codecov Report

Attention: Patch coverage is 88.59649% with 13 lines in your changes missing coverage. Please review.

Project coverage is 90.09%. Comparing base (8fb169e) to head (44de5ea).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...logging/otlp/internal/writer/StreamJsonWriter.java	81.25%	5 Missing and 1 partial ⚠️
...logging/otlp/internal/writer/LoggerJsonWriter.java	78.94%	4 Missing ⚠️
...tlp/internal/logs/OtlpStdoutLogRecordExporter.java	92.85%	1 Missing and 1 partial ⚠️
...logging/otlp/OtlpJsonLoggingLogRecordExporter.java	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #6675   +/-   ##
=========================================
  Coverage     90.09%   90.09%           
- Complexity     6390     6423   +33     
=========================================
  Files           711      717    +6     
  Lines         19333    19422   +89     
  Branches       1891     1894    +3     
=========================================
+ Hits          17418    17499   +81     
- Misses         1335     1341    +6     
- Partials        580      582    +2     

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

[zeitlinger]

@jack-berg can you take a look?

[jack-berg]

Thanks for breaking out - this was more manageable to review.

[zeitlinger]

@jack-berg thanks for the thorough review - I hope I've addressed everything

(build fails because of link checker)

[jack-berg]

We've got to get the delegation model right. We have a new OtlpStdoutLogRecordExporter / OtlpStdOutLogRecordExporterBuilder which we think is going to be the standard going forward, but is internal and experimental for this. This supports a variety of configuration options, include the ability to specify your own output stream and wether you want the "wrapper json object". The old OtlpJsonLoggingLogRecordExporter is just a OtlpStdoutLogRecordExporter with a very specific configuration which is non-configurable. This means that OtlpJsonLoggingLogRecordExporter should delegate to OtlpStdoutLogRecordExporter, since OtlpStdoutLogRecordExporter is a superset of the capabilities of OtlpJsonLoggingLogRecordExporter.

Its flipped right now and I think its requiring some wonky stuff with reflection which makes this more complicated that it needs to be. This is hard to communicate via comments, so I sketched it out quickly in a commit. Note the tests don't pass yet, but I think those have an unnecessary abstract class, as I've mentioned in comments.

jack-berg@7e40c2e

[zeitlinger]

Its flipped right now and I think its requiring some wonky stuff with reflection which makes this more complicated that it needs to be.

I really like the solution you found! I'll use it. 💯

Note the tests don't pass yet

You can't set the builder to use a logger.
The tests pass once setLogger is added.

[zeitlinger]

@jack-berg all suggestions incorporated now 😄

[jack-berg]

Looks pretty good! Smaller number of lingering questions. I made it all the way through the code this time so I think this is likely the last set of comments to resolve.

[jack-berg]

What's going on here? What close are we trying to ignore?

Also, what do you think in general about the resource management of the output stream? Should OtlpStdoutLogRecordExporter be responsible for calling OutputStream.close()? I think probably yes, since otherwise it would be complicated for a user to detect shutdown and close the output stream themselves. On the otherhand, closing System.out is not correct. I wonder if we need a boolean configuration option to indicate whether the OutputStream should be closed on LogRecordExporter#shutdown():

setOutputStream(os, true);

[zeitlinger]

very good point - there's not even a test for using a file.

[zeitlinger]

turned out that we can skip the boolean flag if we just call flush in close

[jack-berg]

Flushing on close will ensure that any unwritten bytes are flushed to the outputstream, but doesn't allow any outputstream resources to be released on shutdown.

[zeitlinger]

we could call close on shutdown instead - but maybe that's too surprising

another option is that we specifically check for System.out and System.err - and only then not call close (I like that).

[jack-berg]

we could call close on shutdown instead - but maybe that's too surprising

That was my point with adding a boolean on setOutputStream where the user could specify whether they want to be responsible for closing the stream or if they want the exporter to be responsible.

another option is that we specifically check for System.out and System.err

True.. That would probably cover most of the cases. But I wonder if there are other cases where you would want to skip closing the OutputStream. We could always start with this suggestion and later add a boolean overload to setOutputStream if needed.

[zeitlinger]

another option is that we specifically check for System.out and System.err

I implemented this now

[jack-berg]

I see code in IgnoreCloseOutputStream#close() which checks if the stream is System.out or System.err, but I don't see where close() is being called anywhere in OtlpStdoutLogRecordExporter#shutdown().

Also, we should add javadoc to OtlpStdoutLogRecordExporterBuilder#setOutput(OutputStream) which indicates that the stream will be closed if its not System.out or System.err.

[zeitlinger]

Great catch - I also found a better way to implement this I hope.

I couldn't find where flush is being called - and if I need to flush the stream manually.

[zeitlinger]

flush is called by jackson (https://fasterxml.github.io/jackson-core/javadoc/2.7/com/fasterxml/jackson/core/JsonGenerator.Feature.html#FLUSH_PASSED_TO_STREAM) - but it seems that

opentelemetry-java/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/LogRecordExporter.java

Line 72 in 8a917e0

CompletableResultCode flush();

is unused

[zeitlinger]

@jack-berg please have another look

if you're ok with a separate experimental module, let me know which module would be appropriate

[zeitlinger]

@jack-berg please have another look 😄

[jack-berg]

Blocking on this comment: #6675 (comment)

Otherwise, just a few nits.

[zeitlinger]

Blocking on this comment: #6675 (comment)

Otherwise, just a few nits.

blocker is resolved @jack-berg 😄

[jack-berg]

Some nits, but 👍

[zeitlinger]

thanks @jack-berg - should I add the other signals next, or the memory mode for logs?

[jack-berg]

No preference from me