Add LogAppender for log crate

[cijothomas]

Initial Version of Log Appender for the Log crate.
TODO: The appender is now dependent on the SDK, which should be fixed to only depend on the API Done with help from @TommyCpp
TODO: Decide on naming of the crate. The name looks good based on feedback from the folks attending SIG meeting on 5/23.

Does not support the structured log as it requires unstable features. Structured logs can be added as a follow up PR. If the overall structure is good, I'd send PR to add similar appenders for other logging libraries, specifically slog, and tracing.

Update:
Based on discussion in this PR, this create is temporarily hosted in the main opentelemetry rust repo. This would be moved to a opentelemetry owned contrib repo (once is it created), or, even to https://github.com/rust-lang/log if the maintainers agree.

[codecov]

Codecov Report

Patch coverage: 2.4% and project coverage change: -0.1 ⚠️

Comparison is base (53cb06e) 50.8% compared to head (cc7d3a9) 50.7%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1071     +/-   ##
=======================================
- Coverage   50.8%   50.7%   -0.1%     
=======================================
  Files        165     166      +1     
  Lines      19751   19781     +30     
=======================================
+ Hits       10037   10038      +1     
- Misses      9714    9743     +29     

Impacted Files	Coverage Δ
opentelemetry-sdk/src/logs/config.rs	0.0% <0.0%> (ø)
opentelemetry-sdk/src/logs/log_emitter.rs	0.0% <0.0%> (ø)
opentelemetry-appender-log/src/lib.rs	3.2% <3.2%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[djc]

It's unclear to me why there's a need for a new crate -- what will this crate contain, and why is it out of place in any of the existing crates?

[vibhavp]

I had created a log-rs adaptor for otel logs, here: https://github.com/vibhavp/log-rs-opentelemetry. The code is similar to this crate, with a few improvements. Would it be possible to merge the two?

That being said, should this be maintained under opentelemetry? For instance, the tracing adaptor for opentelemetry-rust is at https://github.com/tokio-rs/tracing-opentelemetry, and it would be better if we followed a similar pattern, making this its own repository.

Additionally, we should offer additional flexibility to include the trace context in records created by this adaptor. tracing is the de-facto tracing library for Rust, which uses 64-bit span IDs which are then duct-taped to OpenTelemetry's 128-bit IDs through OpenTelemetrySpanExt. As a result, opentelemetry's Context cannot be used to extract the current trace context, which is what the Logger uses.

[cijothomas]

It's unclear to me why there's a need for a new crate -- what will this crate contain, and why is it out of place in any of the existing crates?

what will this crate contain

This is the Logging Bridge (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/glossary.md#log-appender--bridge) for the Log crate. This is a new crate, because this was made possible only recently with this PR : #788, which provided the Bridge API and the SDK side, and this is the 1st Appender/Bridge leveraging it. (I have mentioned the interest to make 2 more bridges, for slog and tracing as well in yesterday's SIG meeting.)

why is it out of place in any of the existing crates?

I cannot see any existing components where this can be put. Do you have any particular create in mind?
I would have preferred to make this in the contrib repo, but was told this repo is currently okay, and could be moved out once contrib repo is created (#841 (comment)).

[cijothomas]

I had created a log-rs adaptor for otel logs, here: https://github.com/vibhavp/log-rs-opentelemetry. The code is similar to this crate, with a few improvements. Would it be possible to merge the two?

That being said, should this be maintained under opentelemetry? For instance, the tracing adaptor for opentelemetry-rust is at https://github.com/tokio-rs/tracing-opentelemetry, and it would be better if we followed a similar pattern, making this its own repository.

Additionally, we should offer additional flexibility to include the trace context in records created by this adaptor. tracing is the de-facto tracing library for Rust, which uses 64-bit span IDs which are then duct-taped to OpenTelemetry's 128-bit IDs through OpenTelemetrySpanExt. As a result, opentelemetry's Context cannot be used to extract the current trace context, which is what the Logger uses.

Happy to get improvements to this! (This PR is just to get something basic working, will be iterating to improve it to add more features like the ones from your repo!)

The question of whether something should be owned by OpenTelemetry - Most other languages keep these in their contrib repos. Some languages like .NET decided to maintain some in the main repo itself. I am okay to move this to contrib repo, once it is available. Based on initial discussion in previous SIG meetings, it was agreed to host in the main repo, and move it out to contrib once ready.

The tracing-opentelemetry repo is not under CNCF/OpenTelemetry org. This topic was discussed in yesterday's SIG call too, and the consensus was to host the tracing bridge here to begin with. Once the components are here, we can evaluate potentially lifting moving it out elsewhere/merging with existing tracing-opentelemetry repo. (That crete already converts events as SpanEvents, but the logging bridge would convert them to OTel logs, so there are issues like duplication, context stamping etc. that need to be sorted out.)

[cijothomas]

Additionally, we should offer additional flexibility to include the trace context in records created by this adaptor. tracing is the de-facto tracing library for Rust, which uses 64-bit span IDs which are then duct-taped to OpenTelemetry's 128-bit IDs through OpenTelemetrySpanExt. As a result, opentelemetry's Context cannot be used to extract the current trace context, which is what the Logger uses.

Agree this is an issue. If OpenTelemetry's TraceContext cannot be used for logs, then that is a bigger issue. We have some discussions already going on about OTel Tracing vs Tokio Tracing (#1032)

Given the extreme popularity of tracing,tracing-opentelemetry, we definitely need to make it work for

1. User who rely on OpenTelemetry Tracing API for distributed tracing.
2. User who rely on Tokio/Tracing API for distributed tracing.

@lalitb @TommyCpp - This is something we discussed in the SIG call too. Once we get the basic appender for tracing working, we can list all the issues and then triage/fix them.

[TommyCpp]

small nits but overall looks good!

[TommyCpp]

Can we move this example to opentelemetry-appender-log folder? The examples is used to demonstrate the how to use this bridge after all and we already have too many examples in top-level example folder

[cijothomas]

Though I agree it doesn't look good to have too many top-level examples, I believe this one is best served in the top-level.

1. It should be okay to have a basic "hello world" style example for each of the signals (metrics, traces already exist) in the top level folder.
2. I can do some cleanup of the existing top-level examples to trim them down.

Thoughts?

[Robo210]

The tracing-opentelemetry repo is not under CNCF/OpenTelemetry org

This doesn't seem like a good reason to duplicate the work they have already done instead of extend it over in their repo. Competing bridges will just bring about confusion and ill-will between projects, and unifying them later will be infinitely harder than not forking the community to begin with.

Plus I personally still don't understand what any of the benefits are for having the code live here vs. there. In the end everyone gets it from crates.io just the same.

[djc]

This doesn't seem like a good reason to duplicate the work they have already done instead of extend it over in their repo. Competing bridges will just bring about confusion and ill-will between projects, and unifying them later will be infinitely harder than not forking the community to begin with.

Plus I personally still don't understand what any of the benefits are for having the code live here vs. there. In the end everyone gets it from crates.io just the same.

Strong +1 from me.

[cijothomas]

The tracing-opentelemetry repo is not under CNCF/OpenTelemetry org

This doesn't seem like a good reason to duplicate the work they have already done instead of extend it over in their repo. Competing bridges will just bring about confusion and ill-will between projects, and unifying them later will be infinitely harder than not forking the community to begin with.

Plus I personally still don't understand what any of the benefits are for having the code live here vs. there. In the end everyone gets it from crates.io just the same.

@djc @Robo210
Repasting the below from my earlier reply.

"The tracing-opentelemetry repo is not under CNCF/OpenTelemetry org. This topic was discussed in yesterday's SIG call too, and the consensus was to host the tracing bridge here to begin with. Once the components are here, we can evaluate potentially lifting moving it out elsewhere/merging with existing tracing-opentelemetry repo. (That crete already converts events as SpanEvents, but the logging bridge would convert them to OTel logs, so there are issues like duplication, context stamping etc. that need to be sorted out.)"

I do not have strong preference if the tracing bridge (for logs) should live in OpenTelemetry repo, or live along with the tracing-opentelemetry (which is currently only for traces). I will start a discussion with the maintainers of the tracing-opentelemetry repo to see if they are happy to host it there, and update back.

@djc The Logging Bridge PR just got merged and needs more polishing before it can be published and leveraged by outside repos. Would you allow this PR (and a similar appender for tracing logs) to proceed in this repo for now, while I gauge the opinions from the tracing-opentelemetry maintainers (to host it there once we have the logging bridge crates published), so that the logging work is unblocked ?

[cijothomas]

The tracing-opentelemetry repo is not under CNCF/OpenTelemetry org

This doesn't seem like a good reason to duplicate the work they have already done instead of extend it over in their repo. Competing bridges will just bring about confusion and ill-will between projects, and unifying them later will be infinitely harder than not forking the community to begin with.
Plus I personally still don't understand what any of the benefits are for having the code live here vs. there. In the end everyone gets it from crates.io just the same.

@djc @Robo210 Repasting the below from my earlier reply.

"The tracing-opentelemetry repo is not under CNCF/OpenTelemetry org. This topic was discussed in yesterday's SIG call too, and the consensus was to host the tracing bridge here to begin with. Once the components are here, we can evaluate potentially lifting moving it out elsewhere/merging with existing tracing-opentelemetry repo. (That crete already converts events as SpanEvents, but the logging bridge would convert them to OTel logs, so there are issues like duplication, context stamping etc. that need to be sorted out.)"

I do not have strong preference if the tracing bridge (for logs) should live in OpenTelemetry repo, or live along with the tracing-opentelemetry (which is currently only for traces). I will start a discussion with the maintainers of the tracing-opentelemetry repo to see if they are happy to host it there, and update back.

@djc The Logging Bridge PR just got merged and needs more polishing before it can be published and leveraged by outside repos. Would you allow this PR (and a similar appender for tracing logs) to proceed in this repo for now, while I gauge the opinions from the tracing-opentelemetry maintainers (to host it there once we have the logging bridge crates published), so that the logging work is unblocked ?

Tagging all maintainers, @jtescher @djc @TommyCpp @hdost to help unblock. (Sorry for pushing, I am trying to get this through (and similar one for tracing), so we can start testing the full logging story with OTel Rust). As mentioned in the earlier comment, happy to move the tracing appender to tracing-opentelemetry repo, once the basic version is ready, and we publish api/sdk that support logs to crates.io.

[jtescher]

I think this is ok to move forward as long as it is clear the intent is to move it to the contrib repo once available. Could be clearer in that sense if this was simply added to the current opentelemetry-contrib crate for now, but doesn't seem urgent enough to block.

[hdost]

I think the intent here is not to step on any toes, but make some progress. Concur with @jtescher

[cijothomas]

@djc @Robo210 I have opened #1080 to track the work for log-appender for tracing. I hope your concerns are addressed correctly in that issue. If not, lets use #1080 to continue that discussion.

Thanks everyone else for reviewing!

@TommyCpp Please let me know if the top level example is okay (i replied to your comment), and if yes, please merge. Thank you.

[djc]

I don't want to block this, just provide guidance on the right approach to move forward. Honestly it's not clear to me why we need a bridge for the log crate (note that the crate is called "log", not "Log", so if the title is referring to that crate, that is confusing to me) when tracing already creates bridging functionality for the log crate natively.

[KodrAus]

👋 I'm currently helping maintain the log crate. I've been tinkering with the SDK independently and just stumbled upon this issue and thought I'd chime in.

This would be moved to a opentelemetry owned contrib repo (once is it created), or, even to https://github.com/rust-lang/log if the maintainers agree.

A contrib repo may be a good start because log is currently in the Rust project's organization, so would probably need some input from the wider Libraries team before adopting anything new.

Honestly it's not clear to me why we need a bridge for the log crate ... when tracing already creates bridging functionality for the log crate natively.

I think creating a bridge natively for log is reasonable even though tracing already also integrates with log. I wouldn't expect tracing to do that native integration indefinitely, especially as it's grown in usage. I think that was necessary to bootstrap something new, but I wouldn't think it unreasonable for them even to drop native log support in their 0.2.x release. The tracing maintainers may have a totally different view though.

[TommyCpp]

Looks good! I can start cleaning the top level examples

[davidbarsky]

I think creating a bridge natively for log is reasonable even though tracing already also integrates with log. I wouldn't expect tracing to do that native integration indefinitely, especially as it's grown in usage. I think that was necessary to bootstrap something new, but I wouldn't think it unreasonable for them even to drop native log support in their 0.2.x release. The tracing maintainers may have a totally different view though.

To be honest, I don't see a world where we drop support for log. Doing so will reduce the utility of both log and tracing and that would be a net-loss for the entire Rust ecosystem.