Change to owned LoggerProvider

[hdost]

The interface to an item shouldn't take an inner value since it's considered inner, this also allows for further optimizations in the future as it hides the complexity from the user.

Relates #1209

Changes

Please provide a brief description of the changes here.

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8688e7e) 61.8% compared to head (18ee088) 61.8%.

Files	Patch %	Lines
opentelemetry-sdk/src/logs/log_emitter.rs	90.9%	1 Missing ⚠️
opentelemetry/src/logs/noop.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #1455   +/-   ##
=====================================
  Coverage   61.8%   61.8%           
=====================================
  Files        144     144           
  Lines      19810   19805    -5     
=====================================
- Hits       12256   12254    -2     
+ Misses      7554    7551    -3     

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

[lalitb]

Trying to understand more here - so with this PR, each logger holds its own complete instance of LoggerProvider, which would be different from the other loggers, and also different from the global LoggerProvider. And will calling shutdown on the global loggerProvider ensure that the logs through these loggers are not exported ?

Also, would there be increased memory usage if many Logger instances are created, as each carries a full copy of the LoggerProvider and its internal state.

Also, please change the PR title to reflect LoggerProvider instead of TracerProvider :)

[hdost]

Trying to understand more here - so with this PR, each logger holds its own complete instance of LoggerProvider, which would be different from the other loggers, and also different from the global LoggerProvider. And will calling shutdown on the global loggerProvider ensure that the logs through these loggers are not exported ?

The inner is still the same, so it should, I'll double check that there's a test.

Also, would there be increased memory usage if many Logger instances are created, as each carries a full copy of the LoggerProvider and its internal state.

It won't be a full copy, the inner is an Arc, so the outer would be an additional pointer memory wise.

Also, please change the PR title to reflect LoggerProvider instead of TracerProvider :)

Done 😅

[lalitb]

It won't be a full copy, the inner is an Arc, so the outer would be an additional pointer memory wise.

Ah good point. I didn't realize that inner is Arc type, and since there is no owned trait implementation, the to_owned() will fallback to default clone, and so all outer loggerprovider instances would share the single inner instance. As @TommyCpp suggested, it might be more transparent and clearer to use self.clone() explicitly in this context.

[lalitb]

And will calling shutdown on the global loggerProvider ensure that the logs through these loggers are not exported ?

It would be good to validate this use-case. It seems shutdown_logger_provider will invoke shutdown on the LogProcessors when the inner instance is dropped. And now if any of the loggers have it's reference in separate thread, the shutdown will not be invoked. One option can be to add a new LoggerProvider::Shutdown() method, and explicitly invoke it from within shutdown_logger_provider . This new method will invoke shutdown on all processors, and so ensure that all the existing logs are flushed. And no new logs can be exported with processors in shutdown state.

[TommyCpp]

Hmm I think this PR is the same as #1446 ?

[hdost]

Hmm I think this PR is the same as #1446 ?

Yes it's meant to be an alternative because right now the initial interface requires passing the under which is inherently leaky as an interface. This aims to remove the necessity.

[lalitb]

Surprisingly, this PR has better perf numbers over #1446, as evident from the stress results. I initially thought both should have similar stats.

/home/labhas/opentelemetry-rust/stress$ cargo run --bin logs --release

#1455 (current PR):
Number of threads: 8
Throughput: 20,641,400 iterations/sec
Throughput: 20,336,600 iterations/sec

#1446:
Number of threads: 8
Throughput: 14,935,400 iterations/sec
Throughput: 15,633,600 iterations/sec

#main branch:
Number of threads: 8
Throughput: 11,473,200 iterations/sec
Throughput: 11,909,200 iterations/sec

[hdost]

That's a nice benefit as well. I believe it's because we're really doing less things in this one.

Side note: I really would love to run whatever machines y'all are running because my same stress benchmarks never get as good of results. Maybe after the next refresh for my laptop 😅

[lalitb]

And will calling shutdown on the global loggerProvider ensure that the logs through these loggers are not exported ?

It would be good to validate this use-case. It seems shutdown_logger_provider will invoke shutdown on the LogProcessors when the inner instance is dropped. And now if any of the loggers have it's reference in separate thread, the shutdown will not be invoked. One option can be to add a new LoggerProvider::Shutdown() method, and explicitly invoke it from within shutdown_logger_provider . This new method will invoke shutdown on all processors, and so ensure that all the existing logs are flushed. And no new logs can be exported with processors in shutdown state.

Thinking about this scenario again, it seems that the potential issue of leaking providers might not be as significant if we can document it clearly to explain the risk. Specifically, providers could be leaked if any of the tasks indefinitely retain their reference, which can result in a shutdown not triggering.

[cijothomas]

Hmm I think this PR is the same as #1446 ?

Yes it's meant to be an alternative because right now the initial interface requires passing the under which is inherently leaky as an interface. This aims to remove the necessity.

Just catching up on PRs! Could you describe any pros/cons of this vs the alternate in the PR desc? Also, if feasible, can you add the stress test numbers you are observing. I am not seeing the same gains as Lalit has posted above, so wondering what is the actual gain! (i'll try to run more tests and post back as well soon)

[lalitb]

My earlier results were on the Azure dev box, but I can also see better perf on my Windows desktop with WSL2:

#main branch:
Running /home/labhas/obs/ot/opentelemetry-rust/target/release/logs
Number of threads: 10
Throughput: 9,659,000 iterations/sec
Throughput: 9,166,800 iterations/sec
Throughput: 9,068,400 iterations/sec
Throughput: 9,013,600 iterations/sec

#this PR:
Running /home/labhas/obs/ot/opentelemetry-rust/target/release/logs
Number of threads: 10
Throughput: 28,168,200 iterations/sec
Throughput: 26,013,800 iterations/sec
Throughput: 23,196,200 iterations/sec
Throughput: 24,413,400 iterations/sec

[hdost]

Are we still thinking that we need to have the testing before this gets merged?

I'm trying to thing what would be a valid testing scenario that we'd want to run.

[cijothomas]

Are we still thinking that we need to have the testing before this gets merged?

I'm trying to thing what would be a valid testing scenario that we'd want to run.

From SIG Call:
We don't need to block the PR for tests - a targetted test can be added as a followup.
Scenario: #1455 (comment)
There are other issues reporting about shutdown scenarios, maybe we need a comprehensive testing targetting shutdowns, separate from this PR.

[cijothomas]

LGTM.
Maybe good to add changelog entry to the sdk.
We can definitely follow up to do targeted tests for shutdown part.

[cijothomas]

I ran stress test for logs after modifying the no-op processor to return false for event_enabled.
main branch:

Number of threads: 8
Throughput: 75,537,000 iterations/sec
Throughput: 77,545,600 iterations/sec

This pr:

Number of threads: 8
Throughput: 579,008,400 iterations/sec
Throughput: 586,900,800 iterations/sec

On the other hand, if using a no-op loglayer with event_enabled returning false, the same test gives below:

Number of threads: 8
Throughput: 1,106,156,200 iterations/sec
Throughput: 1,069,867,600 iterations/sec

This test shows that OTel still has a lot more room for further optimizations. Those can be explored in the future. This PR already boosts the perf significantly!