test(storage): join metrics reader threads before tests exit

google/cloud/storage/internal/grpc/channel_metrics_test.cc

@@ -29,6 +29,7 @@
 #include "google/cloud/testing_util/scoped_environment.h"
 #include <gmock/gmock.h>
 #include <opentelemetry/sdk/metrics/export/metric_producer.h>
+#include <opentelemetry/sdk/metrics/meter_provider.h>
 #include <opentelemetry/sdk/metrics/push_metric_exporter.h>
 #include <opentelemetry/sdk/resource/resource.h>
 #include <atomic>
@@ -83,6 +84,33 @@ class MockPushMetricExporter
   // NOLINTEND(bugprone-exception-escape)
 };
 
+/// Shuts down a `MeterProvider` when the test ends, even after a failed
+/// `ASSERT_*`.
+///
+/// `EnableGrpcMetricsImpl()` stores the provider in process-wide singletons
+/// that are never destroyed. Calling `Shutdown()` joins the provider's
+/// periodic reader thread before the test exits, so a background collection
+/// cannot race with `exit()` destroying OpenTelemetry's static objects.
+class ScopedProviderShutdown {
+ public:
+  explicit ScopedProviderShutdown(
+      std::shared_ptr<opentelemetry::metrics::MeterProvider> provider)
+      : provider_(std::move(provider)) {}
+  ~ScopedProviderShutdown() {
+    if (!provider_) return;
+    // The API `MeterProvider` interface has no `Shutdown()`, and
+    // `FindMeterProvider()` only returns `sdk::metrics::MeterProvider`
+    // instances created by `MakeGrpcMeterProvider()`.
+    static_cast<opentelemetry::sdk::metrics::MeterProvider*>(provider_.get())
+        ->Shutdown();
+  }
+  ScopedProviderShutdown(ScopedProviderShutdown const&) = delete;
+  ScopedProviderShutdown& operator=(ScopedProviderShutdown const&) = delete;
+
+ private:
+  std::shared_ptr<opentelemetry::metrics::MeterProvider> provider_;
+};
+
 auto constexpr kExportInterval = std::chrono::milliseconds(50);
 auto constexpr kExportTimeout = std::chrono::milliseconds(25);
 
@@ -271,7 +299,6 @@ TEST(ChannelMetrics, UnknownAuthorityYieldsEmptyCallback) {
 /// that sets `EnableGrpcMetricsOption(false)` must still record nothing,
 /// even though `FindMeterProvider()` would succeed.
 TEST(ChannelMetrics, RegisteredAuthorityHonorsOption) {
-  GTEST_SKIP() << "Skipping flaky test, see #16587";
   // The registry is process-wide, so use an authority unique to this test.
   std::string const authority = "test-only-registered.example.com";
   ScopedEnvironment const env("GOOGLE_CLOUD_PROJECT", std::nullopt);
@@ -286,7 +313,10 @@ TEST(ChannelMetrics, RegisteredAuthorityHonorsOption) {
       opentelemetry::sdk::resource::Resource::Create({}), base);
   ASSERT_TRUE(config.has_value());
   EnableGrpcMetricsImpl(*std::move(config));
-  ASSERT_TRUE(FindMeterProvider(authority));
+  std::shared_ptr<opentelemetry::metrics::MeterProvider> const provider =
+      FindMeterProvider(authority);
+  ScopedProviderShutdown const provider_shutdown(provider);
+  ASSERT_TRUE(provider);
 
   struct TestCase {
     bool enabled;

google/cloud/storage/internal/grpc/metrics_exporter_impl_test.cc

@@ -25,12 +25,14 @@
 #include "google/cloud/testing_util/chrono_output.h"
 #include "google/cloud/testing_util/scoped_environment.h"
 #include <gmock/gmock.h>
+#include <opentelemetry/sdk/metrics/meter_provider.h>
 #include <chrono>
 #include <future>
 #include <memory>
 #include <optional>
 #include <string>
 #include <thread>
+#include <utility>
 #include <vector>
 
 namespace google {
@@ -64,6 +66,33 @@ auto TestOptions(Options opts = {}) {
   return DefaultOptionsGrpc(std::move(opts));
 }
 
+/// Shuts down a `MeterProvider` when the test ends, even after a failed
+/// `ASSERT_*`.
+///
+/// `EnableGrpcMetricsImpl()` stores the provider in process-wide singletons
+/// that are never destroyed. Calling `Shutdown()` joins the provider's
+/// periodic reader thread before the test exits, so a background collection
+/// cannot race with `exit()` destroying OpenTelemetry's static objects.
+class ScopedProviderShutdown {
+ public:
+  explicit ScopedProviderShutdown(
+      std::shared_ptr<opentelemetry::metrics::MeterProvider> provider)
+      : provider_(std::move(provider)) {}
+  ~ScopedProviderShutdown() {
+    if (!provider_) return;
+    // The API `MeterProvider` interface has no `Shutdown()`, and
+    // `FindMeterProvider()` only returns `sdk::metrics::MeterProvider`
+    // instances created by `MakeGrpcMeterProvider()`.
+    static_cast<opentelemetry::sdk::metrics::MeterProvider*>(provider_.get())
+        ->Shutdown();
+  }
+  ScopedProviderShutdown(ScopedProviderShutdown const&) = delete;
+  ScopedProviderShutdown& operator=(ScopedProviderShutdown const&) = delete;
+
+ private:
+  std::shared_ptr<opentelemetry::metrics::MeterProvider> provider_;
+};
+
 TEST(GrpcMetricsExporter, DisabledBecauseOption) {
   auto config = MakeMeterProviderConfig(
       FullResource(),
@@ -232,7 +261,6 @@ TEST(MakeMeterProviderConfigTest, EmptyExcludedLabels) {
 /// provider, otherwise those instruments would be split across two exporters.
 /// The authority is unique to this test because the registry is process-wide.
 TEST(GrpcMetricsExporter, FindMeterProviderAfterEnable) {
-  GTEST_SKIP() << "Skipping flaky test, see #16587";
   std::string const authority = "test-only-find-provider.example.com";
   std::optional<ExporterConfig> config = MakeMeterProviderConfig(
       FullResource(), TestOptions().set<AuthorityOption>(authority));
@@ -242,6 +270,7 @@ TEST(GrpcMetricsExporter, FindMeterProviderAfterEnable) {
   EnableGrpcMetricsImpl(*config);
   std::shared_ptr<opentelemetry::metrics::MeterProvider> const provider =
       FindMeterProvider(authority);
+  ScopedProviderShutdown const provider_shutdown(provider);
   EXPECT_THAT(provider, NotNull());
 
   EnableGrpcMetricsImpl(*config);
@@ -252,7 +281,6 @@ TEST(GrpcMetricsExporter, FindMeterProviderAfterEnable) {
 /// @test Verify concurrent callers for the same authority all observe the
 /// initialized provider.
 TEST(GrpcMetricsExporter, ConcurrentEnableFindsSameProvider) {
-  GTEST_SKIP() << "Skipping flaky test, see #16587";
   std::string const authority = "test-only-concurrent-provider.example.com";
   std::optional<ExporterConfig> const config = MakeMeterProviderConfig(
       FullResource(), TestOptions().set<AuthorityOption>(authority));
@@ -274,6 +302,7 @@ TEST(GrpcMetricsExporter, ConcurrentEnableFindsSameProvider) {
   }
   start_promise.set_value();
   for (auto& t : threads) t.join();
+  ScopedProviderShutdown const provider_shutdown(FindMeterProvider(authority));
 
   EXPECT_THAT(providers[0], NotNull());
   for (int i = 1; i != kThreadCount; ++i) {