Reviewer comments

src/cpp/ext/otel/otel_plugin.cc

@@ -575,6 +575,7 @@ OpenTelemetryPluginImpl::~OpenTelemetryPluginImpl() {
         [](const std::unique_ptr<opentelemetry::metrics::Histogram<double>>&) {
         },
         [](const std::unique_ptr<CallbackGaugeState<int64_t>>& state) {
+          CHECK(state->caches.empty());
           if (state->ot_callback_registered) {
             state->instrument->RemoveCallback(
                 &CallbackGaugeState<int64_t>::CallbackGaugeCallback,
@@ -583,6 +584,7 @@ OpenTelemetryPluginImpl::~OpenTelemetryPluginImpl() {
           }
         },
         [](const std::unique_ptr<CallbackGaugeState<double>>& state) {
+          CHECK(state->caches.empty());
           if (state->ot_callback_registered) {
             state->instrument->RemoveCallback(
                 &CallbackGaugeState<double>::CallbackGaugeCallback,

test/cpp/ext/otel/otel_plugin_test.cc

@@ -1715,8 +1715,8 @@ TEST_F(OpenTelemetryPluginNPCMetricsTest, InstrumentsEnabledTest) {
   EXPECT_FALSE(stats_plugins.IsInstrumentEnabled(counter_handle));
 }
 
-typedef OpenTelemetryPluginNPCMetricsTest
-    OpenTelemetryPluginCallbackMetricsTest;
+using OpenTelemetryPluginCallbackMetricsTest =
+    OpenTelemetryPluginNPCMetricsTest;
 
 // The callback minimal interval is longer than the OT reporting interval, so we
 // expect to collect duplicated (cached) values.
@@ -2056,12 +2056,38 @@ TEST_F(OpenTelemetryPluginCallbackMetricsTest, VerifyCallbacksAreCleanedUp) {
   // Verify that callbacks are invoked
   EXPECT_EQ(report_count_1, kIterations);
   EXPECT_EQ(report_count_2, kIterations);
-  // Reset the global stats plugins
+  // Remove one of the callbacks
   registered_metric_callback_1.reset();
+  MetricsCollectorThread new_collector{
+      this,
+      grpc_core::Duration::Milliseconds(100) * grpc_test_slowdown_factor(),
+      kIterations,
+      [&](const absl::flat_hash_map<
+          std::string,
+          std::vector<opentelemetry::sdk::metrics::PointDataAttributes>>&
+              data) { return false; }};
+  new_collector.Stop();
+  EXPECT_EQ(report_count_1, kIterations);      // No change since previous
+  EXPECT_EQ(report_count_2, 2 * kIterations);  // Gets another kIterations
+  // Remove the other callback as well
   registered_metric_callback_2.reset();
+  MetricsCollectorThread new_new_collector{
+      this,
+      grpc_core::Duration::Milliseconds(100) * grpc_test_slowdown_factor(),
+      kIterations,
+      [&](const absl::flat_hash_map<
+          std::string,
+          std::vector<opentelemetry::sdk::metrics::PointDataAttributes>>&
+              data) { return false; }};
+  // We shouldn't get any new callbacks
+  EXPECT_THAT(new_new_collector.Stop(), ::testing::IsEmpty());
+  EXPECT_EQ(report_count_1, kIterations);
+  EXPECT_EQ(report_count_2, 2 * kIterations);
+  // Reset stats plugins as well
   grpc_core::GlobalStatsPluginRegistryTestPeer::
       ResetGlobalStatsPluginRegistry();
-  MetricsCollectorThread new_collector{
+  registered_metric_callback_2.reset();
+  MetricsCollectorThread new_new_new_collector{
       this,
       grpc_core::Duration::Milliseconds(100) * grpc_test_slowdown_factor(),
       kIterations,
@@ -2069,10 +2095,10 @@ TEST_F(OpenTelemetryPluginCallbackMetricsTest, VerifyCallbacksAreCleanedUp) {
           std::string,
           std::vector<opentelemetry::sdk::metrics::PointDataAttributes>>&
               data) { return false; }};
-  // We shouldn't get any new callbacks
-  EXPECT_EQ(new_collector.Stop().size(), 0);
+  // Still no new callbacks
+  EXPECT_THAT(new_new_new_collector.Stop(), ::testing::IsEmpty());
   EXPECT_EQ(report_count_1, kIterations);
-  EXPECT_EQ(report_count_2, kIterations);
+  EXPECT_EQ(report_count_2, 2 * kIterations);
 }
 
 TEST(OpenTelemetryPluginMetricsEnablingDisablingTest, TestEnableDisableAPIs) {