Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

lalitb · 2024-10-11T19:35:51Z

Changes

changes similar to #2195 for TracerProvider

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 · 2024-10-11T19:39:07Z

Codecov Report

Attention: Patch coverage is 96.46018% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.1%. Comparing base (2bb53b4) to head (1272312).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/trace/provider.rs	96.4%	4 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #2197   +/-   ##
=====================================
  Coverage   79.1%   79.1%           
=====================================
  Files        121     121           
  Lines      21084   21180   +96     
=====================================
+ Hits       16678   16772   +94     
- Misses      4406    4408    +2

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

opentelemetry-sdk/src/trace/provider.rs

cijothomas

Lets make it similar to #2195

…TracerProviderInner::Shutdown

…tb/opentelemetry-rust into tracer-provider-drop-shutdown-check

lalitb · 2024-10-14T17:01:59Z

Lets make it similar to #2195

Done.

opentelemetry-sdk/src/trace/provider.rs

utpilla · 2024-10-15T23:45:52Z

opentelemetry/src/trace/mod.rs

@@ -200,6 +200,10 @@ pub enum TraceError {
    #[error("Exporting timed out after {} seconds", .0.as_secs())]
    ExportTimedOut(time::Duration),

+    /// already shutdown error
+    #[error("{0} already shutdown")]
+    AlreadyShutdown(String),


Do we expect to use this variant for anything other than TracerProvider?

Yes, thought to use them for the processors and exporters too. But I believe we can customize it later if required. For now, made it static for TracerProvider.

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

opentelemetry-sdk/src/trace/provider.rs

cijothomas · 2024-10-17T18:52:08Z

opentelemetry-sdk/src/trace/provider.rs

+///
+/// ## Cloning and Shutdown
+///
+/// The `TracerProvider` is designed to be lightweight and clonable. Cloning a `TracerProvider`


I don't think TracerProvider is lightweight. It is pretty heavy, and we expect user to create it only once. It is correct to mention cloning is cheap as it is just creating a new ref.

Good point. Have updated the comment to remove the lightweight reference.

opentelemetry-sdk/src/trace/provider.rs

cijothomas · 2024-10-17T18:53:59Z

opentelemetry-sdk/src/trace/provider.rs

+///
+/// The `TracerProvider` manages the lifecycle of span processors, which are responsible for
+/// collecting, processing, and exporting spans. To ensure all spans are processed before shutdown,
+/// users can call the [`force_flush`](TracerProvider::force_flush) method at any time to trigger


lets remove force_flush mention here. I have seen many users doing force_flush in their code (and block their threads).. Not sure why, but lets make sure official docs don't recommend it.

Have reworded it so that it doesn't look as recommendation. I think it's better to at-least document since we provide it.

/// ## Span Processing and Force Flush /// /// The `TracerProvider` manages the lifecycle of span processors, which are responsible for /// collecting, processing, and exporting spans. The [`force_flush`](TracerProvider::force_flush) method /// invoked at any time will trigger an immediate flush of all pending spans (if any) to the exporters. /// This will block the user thread till all the spans are passed to exporters

cijothomas · 2024-10-17T18:55:13Z

opentelemetry-sdk/src/trace/provider.rs

-/// `TracerProvider` is lightweight container holding pointers to `SpanProcessor` and other components.
-/// Cloning and dropping them will not stop the span processing. To stop span processing, users
-/// must either call `shutdown` method explicitly, or drop every clone of `TracerProvider`.
+/// `TracerProvider` is a lightweight container holding pointers to `SpanProcessor` and other components.


not introduced in this PR, but advertising TracerProvider as lightweight is incorrect, and can lead to users repeatedly creating them, instead of doing it once.

Have updated the comment to remove the lightweight reference.

cijothomas · 2024-10-17T18:56:07Z

opentelemetry-sdk/src/trace/provider.rs

+
+    #[derive(Debug)]
+    struct CountingShutdownProcessor {
+        shutdown_count: Arc<Mutex<i32>>,


nit: Atomics maybe easier here

…tb/opentelemetry-rust into tracer-provider-drop-shutdown-check

opentelemetry-sdk/src/trace/provider.rs

Co-authored-by: Cijo Thomas <[email protected]>

opentelemetry/src/trace/mod.rs

opentelemetry-sdk/src/trace/provider.rs

utpilla · 2024-10-21T21:00:29Z

opentelemetry-sdk/src/trace/provider.rs

@@ -36,36 +90,60 @@ static NOOP_TRACER_PROVIDER: Lazy<TracerProvider> = Lazy::new(|| TracerProvider
            span_limits: SpanLimits::default(),
            resource: Cow::Owned(Resource::empty()),
        },
+        is_shutdown: AtomicBool::new(true),


Not added in this PR but why have we initialized the no-op tracer provider as an already shut down provider?

I know this wouldn't make much difference in functionality but semantically it would be weird if I call shutdown on the global provider and get an error saying it has already been shut down.

global shutdown_tracer_provider naming is bit confusing. It doesn't invoke shutdown, only decrements the TracerProviderInner reference. Once all the tracers are dropped, the shutdown will get invoked through Drop. Probably release_tracer_provider is better name :)

And the user will never get hold of NOOP_TRACER_PROVIDER instance, or can invoke shutdown on it directly or indirectly. It's just internally used to create the tracer instance after shutdown.

Okay so then we could initialize no-op tracer provider with is_shutdown as false?

Okay so then we could initialize no-op tracer provider with is_shutdown as false?

Making it false fails this test -

opentelemetry-rust/opentelemetry-sdk/src/trace/provider.rs

Line 531 in ea4b5e4

fn test_shutdown_noops() {

And this test make me realise user can access it through tracer.provider(), and can try to invoke shutdown on it. But feel free to raise an issue if there are any improvements here.

opentelemetry-sdk/src/trace/provider.rs

initial commit

32938be

lalitb requested a review from a team as a code owner October 11, 2024 19:35

lalitb changed the title ~~void redundant shutdown in TracerProvider::drop when already shut down~~ Avoid redundant shutdown in TracerProvider::drop when already shut down Oct 11, 2024

Merge branch 'main' into tracer-provider-drop-shutdown-check

01c970b

cijothomas reviewed Oct 11, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

user reusable shutdown

c08c055

cijothomas requested changes Oct 13, 2024

View reviewed changes

lalitb and others added 3 commits October 14, 2024 09:09

Merge branch 'main' into tracer-provider-drop-shutdown-check

82d2598

restructure TracerProviderInner::Drop, TracerProvider::Shutdown, and …

f270dcd

…TracerProviderInner::Shutdown

Merge branch 'tracer-provider-drop-shutdown-check' of github.com:lali…

c8f2166

…tb/opentelemetry-rust into tracer-provider-drop-shutdown-check

utpilla reviewed Oct 15, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 15, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 15, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 15, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Show resolved Hide resolved

utpilla reviewed Oct 15, 2024

View reviewed changes

lalitb and others added 2 commits October 15, 2024 18:33

Update opentelemetry-sdk/src/trace/provider.rs

1a3dd67

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

Update opentelemetry-sdk/src/trace/provider.rs

36012dc

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

cijothomas reviewed Oct 16, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

cijothomas reviewed Oct 16, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

cijothomas reviewed Oct 16, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

lalitb and others added 2 commits October 17, 2024 11:28

fix unit test

0d7d1ab

Merge branch 'main' into tracer-provider-drop-shutdown-check

01234c5

cijothomas reviewed Oct 17, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

cijothomas reviewed Oct 17, 2024

View reviewed changes

update doc

2279f3a

lalitb added 2 commits October 17, 2024 14:55

Merge branch 'tracer-provider-drop-shutdown-check' of github.com:lali…

293120b

…tb/opentelemetry-rust into tracer-provider-drop-shutdown-check

update to atomics

21fa84e

cijothomas reviewed Oct 18, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

lalitb and others added 2 commits October 18, 2024 12:03

Update opentelemetry-sdk/src/trace/provider.rs

6ea4a63

Co-authored-by: Cijo Thomas <[email protected]>

Merge branch 'main' into tracer-provider-drop-shutdown-check

3dbb66e

utpilla reviewed Oct 21, 2024

View reviewed changes

opentelemetry/src/trace/mod.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 21, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Show resolved Hide resolved

utpilla reviewed Oct 21, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 21, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Show resolved Hide resolved

utpilla reviewed Oct 21, 2024

View reviewed changes

lalitb added 3 commits October 22, 2024 11:39

review comment

05b8f59

review comments

74bf489

rename AlreadyShutdown to TracerProviderAlreadyShutdown

556c529

utpilla reviewed Oct 22, 2024

View reviewed changes

opentelemetry-sdk/src/trace/provider.rs Outdated Show resolved Hide resolved

fix

80c97f7

utpilla approved these changes Oct 22, 2024

View reviewed changes

lalitb and others added 4 commits October 22, 2024 15:01

review comments

d3a557f

add test for noop shutdown

14a42cc

Merge branch 'main' into tracer-provider-drop-shutdown-check

2e5899b

Merge branch 'main' into tracer-provider-drop-shutdown-check

1272312

cijothomas approved these changes Oct 23, 2024

View reviewed changes

cijothomas merged commit a47b429 into open-telemetry:main Oct 23, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

lalitb commented Oct 11, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

cijothomas left a comment

lalitb commented Oct 14, 2024

utpilla Oct 15, 2024

lalitb Oct 17, 2024

cijothomas Oct 17, 2024

lalitb Oct 22, 2024 •

edited

Loading

cijothomas Oct 17, 2024

lalitb Oct 17, 2024

cijothomas Oct 17, 2024

lalitb Oct 22, 2024

cijothomas Oct 17, 2024

lalitb Oct 17, 2024

utpilla Oct 21, 2024 •

edited

Loading

lalitb Oct 22, 2024 •

edited

Loading

utpilla Oct 22, 2024

lalitb Oct 22, 2024

Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

Avoid redundant shutdown in TracerProvider::drop when already shut down #2197

Conversation

lalitb commented Oct 11, 2024

Changes

Merge requirement checklist

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

cijothomas left a comment

Choose a reason for hiding this comment

lalitb commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

lalitb Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2024 •

edited

Loading

lalitb Oct 22, 2024 •

edited

Loading

utpilla Oct 21, 2024 •

edited

Loading

lalitb Oct 22, 2024 •

edited

Loading