Skip to content

initial commit of logging support extension#1524

Merged
anuraaga merged 27 commits intoopen-telemetry:masterfrom
zenmoto:logging_extension
Oct 1, 2020
Merged

initial commit of logging support extension#1524
anuraaga merged 27 commits intoopen-telemetry:masterfrom
zenmoto:logging_extension

Conversation

@zenmoto
Copy link
Copy Markdown
Contributor

@zenmoto zenmoto commented Aug 10, 2020

This PR brings a logging delivery API extension. The intent is to allow exporters for existing logging libraries to be built that can then deliver log entries in a way that's consistent with log and tracing support. This PR is infrastructure; if merged, the next PR will be an exporter that uses this infrastructure to deliver logs through the collector via OTLP. The Log4j extension (open-telemetry/opentelemetry-java-instrumentation#735) can then be augmented to optionally deliver logs via this API and export modules for additional libraries can be written.

@jkwatson
Copy link
Copy Markdown
Contributor

Thanks for this contribution.

I'm surprised that we would recommend having the SDK itself handle logging exports, rather than using a more robust log forwarder. Logging can be extremely high volume, and generally has very high availability & delivery guarantees. Are we sure that this is the right solution for OpenTelemetry logging, rather than just using a standard log forwarding solution?

I haven't followed the logging SIG much at all, but I'm surprised that this would be the recommended solution for OTel logging. Can you point me at a document that recommends this approach?

@zenmoto
Copy link
Copy Markdown
Contributor Author

zenmoto commented Aug 11, 2020

I think that we are going to have multiple routes for log delivery- one would be to use existing logging appenders/infrastructure, but adding request correlation fields. We also want to be able to support unified collection through the collector, however, as described in 0092-logs-vision.md.

My intent for this API is to essentially be a minimal middleman that can take logs from user-centric APIs like Log4j/Logback and pass them to an exporter than can handle forwarding to whatever destination, whether it's the collector/OTLP or an existing log aggregator like Fluentd/Logstash/Flume/Splunk HEC/etc.

@trask
Copy link
Copy Markdown
Member

trask commented Aug 11, 2020

Looking forward to this, then I can bring back open-telemetry/opentelemetry-java-instrumentation#803 😄

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 24, 2020

Codecov Report

Merging #1524 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1524   +/-   ##
=========================================
  Coverage     85.36%   85.36%           
  Complexity     1385     1385           
=========================================
  Files           164      164           
  Lines          5445     5445           
  Branches        562      562           
=========================================
  Hits           4648     4648           
  Misses          590      590           
  Partials        207      207           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9bd619...adf4a30. Read the comment docs.

rename Exporter to LogExporter, move to 'export' package
rename LogExporter.accept(data) to LogExporter.export(data)
decrease flush interval from 5000 to 200ms
return CompletableResultCode from LogExporter.shutdown()
change traceId and spanId types to String from byte[]
Comment thread sdk_extensions/logging/support/build.gradle Outdated
@bogdandrutu
Copy link
Copy Markdown
Member

To summarize:

io.opentelemetry.sdk.extensions.logging:

  • LogProcessor

io.opentelemetry.sdk.extensions.logging.data:

  • LogRecord
  • AnyValue
  • any other data class

io.opentelemetry.sdk.extensions.logging.export:

  • BatchLogProcessor
  • any other export related class

Copy link
Copy Markdown
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good starting point. Maybe add a README with a note that this is in alpha stage.

Copy link
Copy Markdown
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tigrannajaryan
Copy link
Copy Markdown
Member

I don't see any unresolved comments. Is there anything else needed before merging this PR?

@anuraaga anuraaga merged commit 64c2b8c into open-telemetry:master Oct 1, 2020
@anuraaga
Copy link
Copy Markdown
Contributor

anuraaga commented Oct 1, 2020

Sorry for the delay, thanks for the hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants