Skip to content

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Jul 2, 2025

Problem

When MeterProvider::Shutdown() is called manually, the destructor still calls Shutdown() again, which causes MeterContext to emit a warning:

[MeterContext::Shutdown] Shutdown can be invoked only once.

This creates unnecessary noise in logs when users properly shut down their meter providers manually before destruction.

Root Cause

The MeterProvider destructor unconditionally calls context_->Shutdown() without checking if shutdown has already been performed:

MeterProvider::~MeterProvider()
{
  if (context_)
  {
    context_->Shutdown();  // Always calls shutdown
  }
}

Solution

This PR implements the same pattern used by other components in the codebase like BatchSpanProcessor and SimpleLogRecordProcessor:

  1. Added IsShutdown() method to MeterContext - Allows checking shutdown state without side effects
  2. Added atomic<bool> is_shutdown_ member - Tracks shutdown state independently of the shutdown latch
  3. Updated MeterProvider destructor - Only calls Shutdown() if not already shut down
  4. Preserved existing behavior - Manual duplicate shutdown calls still emit warnings as expected

Changes

Before:

MeterProvider::~MeterProvider()
{
  if (context_)
  {
    context_->Shutdown();  // Always warns if shutdown was called manually
  }
}

After:

MeterProvider::~MeterProvider()
{
  if (context_ && !context_->IsShutdown())
  {
    context_->Shutdown();  // Only shutdown if not already done
  }
}

Testing

  • ✅ Existing tests pass without destructor warnings
  • ✅ New ShutdownTwice test validates the fix
  • ✅ Manual duplicate shutdown calls still warn appropriately
  • ✅ All metrics tests continue to pass

Before Fix

[Warning] File: .../meter_context.cc:178 [MeterContext::Shutdown] Shutdown can be invoked only once.

After Fix

No warnings from destructor when Shutdown() was called manually.

Fixes #3511.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copy link

linux-foundation-easycla bot commented Jul 2, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Copilot / name: copilot-swe-agent[bot] (7b1872b, 1d9a4de, aff62f5)
  • ✅ login: lalitb / name: Lalit Kumar Bhasin (4de0bd5)
  • ✅ login: lalitb / name: lalitb (7b1872b)
  • ✅ login: ThomsonTan / name: Tom Tan (aff62f5)

Copy link

netlify bot commented Jul 2, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 4de0bd5
🔍 Latest deploy log https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/68753e6e89d1040008f8b1d8

- Add IsShutdown() method to MeterContext
- Add atomic<bool> is_shutdown_ member to track shutdown state
- Update MeterProvider destructor to check IsShutdown() before calling Shutdown()
- Add test case for shutdown behavior
- Follows same pattern as BatchSpanProcessor and SimpleLogRecordProcessor

Co-authored-by: lalitb <[email protected]>
@Copilot Copilot AI changed the title [WIP] Cannot Shutdown() MeterProvider manually without a warning from destructor Fix MeterProvider destructor warning when Shutdown() called manually Jul 2, 2025
@Copilot Copilot AI requested a review from lalitb July 2, 2025 01:26
Copilot finished work on behalf of lalitb July 2, 2025 01:26
Copy link

codecov bot commented Jul 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.97%. Comparing base (62ba4de) to head (4de0bd5).
⚠️ Report is 44 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3513      +/-   ##
==========================================
- Coverage   90.01%   89.97%   -0.03%     
==========================================
  Files         219      219              
  Lines        7044     7047       +3     
==========================================
  Hits         6340     6340              
- Misses        704      707       +3     
Files with missing lines Coverage Δ
sdk/src/metrics/meter_context.cc 78.21% <100.00%> (+0.88%) ⬆️
sdk/src/metrics/meter_provider.cc 87.18% <100.00%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Per previous comments, clarify whether using generative AI is permissible or not.

@marcalff marcalff self-requested a review July 2, 2025 23:31
@ThomsonTan
Copy link
Contributor

Please fix the below format error from CI.

The following files have changes:
sdk/test/metrics/meter_provider_sdk_test.cc
diff --git a/sdk/test/metrics/meter_provider_sdk_test.cc b/sdk/test/metrics/meter_provider_sdk_test.cc
index 2254be0..bf05572 100644
--- a/sdk/test/metrics/meter_provider_sdk_test.cc
+++ b/sdk/test/metrics/meter_provider_sdk_test.cc
@@ -328,12 +328,12 @@ TEST(MeterProvider, ShutdownTwice)
 {
   // Test that calling Shutdown twice doesn't emit warnings
   MeterProvider mp;
-  
+
   // First shutdown should succeed
   EXPECT_TRUE(mp.Shutdown());
-  
+
   // Second shutdown should also succeed without warnings
   EXPECT_TRUE(mp.Shutdown());
-  
+
   // Destructor should not emit warnings either since we already shut down
 }
Error: Process completed with exit code 1.

Copy link
Author

Copilot AI commented Jul 2, 2025

Please fix the below format error from CI.

...

Fixed the formatting error by removing trailing whitespace from the test file. Commit aff62f5

@Copilot Copilot AI requested a review from ThomsonTan July 2, 2025 23:43
Copilot finished work on behalf of ThomsonTan July 2, 2025 23:43
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Blocking due to missing CLA

@marcalff marcalff self-requested a review August 4, 2025 19:37
@marcalff marcalff added the pr:copilot PR generated with AI assistance (Copilot) label Aug 29, 2025
@marcalff
Copy link
Member

/easycla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:copilot PR generated with AI assistance (Copilot)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot Shutdown() MeterProvider manually without a warning from destructor
4 participants