diff --git a/google/cloud/storage/internal/grpc/channel_metrics_test.cc b/google/cloud/storage/internal/grpc/channel_metrics_test.cc index f7f5dd18cf753..45b343d04b018 100644 --- a/google/cloud/storage/internal/grpc/channel_metrics_test.cc +++ b/google/cloud/storage/internal/grpc/channel_metrics_test.cc @@ -29,6 +29,7 @@ #include "google/cloud/testing_util/scoped_environment.h" #include #include +#include #include #include #include @@ -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 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(provider_.get()) + ->Shutdown(); + } + ScopedProviderShutdown(ScopedProviderShutdown const&) = delete; + ScopedProviderShutdown& operator=(ScopedProviderShutdown const&) = delete; + + private: + std::shared_ptr 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 const provider = + FindMeterProvider(authority); + ScopedProviderShutdown const provider_shutdown(provider); + ASSERT_TRUE(provider); struct TestCase { bool enabled; diff --git a/google/cloud/storage/internal/grpc/metrics_exporter_impl_test.cc b/google/cloud/storage/internal/grpc/metrics_exporter_impl_test.cc index c98b02ec41be5..d10c532098e54 100644 --- a/google/cloud/storage/internal/grpc/metrics_exporter_impl_test.cc +++ b/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 +#include #include #include #include #include #include #include +#include #include 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 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(provider_.get()) + ->Shutdown(); + } + ScopedProviderShutdown(ScopedProviderShutdown const&) = delete; + ScopedProviderShutdown& operator=(ScopedProviderShutdown const&) = delete; + + private: + std::shared_ptr 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 config = MakeMeterProviderConfig( FullResource(), TestOptions().set(authority)); @@ -242,6 +270,7 @@ TEST(GrpcMetricsExporter, FindMeterProviderAfterEnable) { EnableGrpcMetricsImpl(*config); std::shared_ptr 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 const config = MakeMeterProviderConfig( FullResource(), TestOptions().set(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) {