From 36556976e5e285a8414bb157fe5e229aa901a4db Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 02:59:23 +0800 Subject: [PATCH 1/4] [CODE HEALTH] Move SDK common and logs test helpers into anonymous namespaces Three misc-use-internal-linkage sites, continuing from #4217 and #4286. global_log_handle_singleton_lifetime_test.cc had an anonymous namespace holding only ConstructChecker(), with GlobalLogHandlerChecker above it and CustomLogHandler below, so the existing namespace is extended over both rather than joined by a second one. That file is a destruction-order test whose checker aborts if the global log handler outlives it, so I built and ran it rather than reasoning about it. The order is unchanged: the handler is destroyed first and the checker's check passes. An anonymous namespace changes linkage, not storage duration, and the function-local static is still constructed on its first call from the test body. logger_provider_set_test.cc gets a namespace around the test body. The warning limit drops by three on both presets, 151 to 148 and 161 to 158. None of these three are ABI gated. Part of #4196 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .github/workflows/clang-tidy.yaml | 4 ++-- .../common/global_log_handle_singleton_lifetime_test.cc | 8 +++++--- sdk/test/logs/logger_provider_set_test.cc | 5 +++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/.github/workflows/clang-tidy.yaml b/.github/workflows/clang-tidy.yaml index 48a3cc8d60..8a206ced36 100644 --- a/.github/workflows/clang-tidy.yaml +++ b/.github/workflows/clang-tidy.yaml @@ -17,9 +17,9 @@ jobs: matrix: include: - cmake_options: all-options-abiv1-preview - warning_limit: 151 + warning_limit: 148 - cmake_options: all-options-abiv2-preview - warning_limit: 161 + warning_limit: 158 env: CC: /usr/bin/clang-22 CXX: /usr/bin/clang++-22 diff --git a/sdk/test/common/global_log_handle_singleton_lifetime_test.cc b/sdk/test/common/global_log_handle_singleton_lifetime_test.cc index 1aac3ee412..a09510416d 100644 --- a/sdk/test/common/global_log_handle_singleton_lifetime_test.cc +++ b/sdk/test/common/global_log_handle_singleton_lifetime_test.cc @@ -11,6 +11,9 @@ #include "opentelemetry/sdk/common/attribute_utils.h" #include "opentelemetry/sdk/common/global_log_handler.h" +namespace +{ + class GlobalLogHandlerChecker { public: @@ -38,14 +41,11 @@ class GlobalLogHandlerChecker }; bool GlobalLogHandlerChecker::custom_handler_destroyed = false; -namespace -{ static GlobalLogHandlerChecker &ConstructChecker() { static GlobalLogHandlerChecker checker; return checker; } -} // namespace class CustomLogHandler : public opentelemetry::sdk::common::internal_log::LogHandler { @@ -112,3 +112,5 @@ TEST(GlobalLogHandleSingletonTest, Lifetime) EXPECT_TRUE(!!handle); } } + +} // namespace diff --git a/sdk/test/logs/logger_provider_set_test.cc b/sdk/test/logs/logger_provider_set_test.cc index aa899bbcbd..bb849549fb 100644 --- a/sdk/test/logs/logger_provider_set_test.cc +++ b/sdk/test/logs/logger_provider_set_test.cc @@ -28,6 +28,9 @@ namespace nostd = opentelemetry::nostd; namespace logs_api = opentelemetry::logs; namespace logs_sdk = opentelemetry::sdk::logs; +namespace +{ + class TestProvider : public LoggerProvider { public: @@ -86,3 +89,5 @@ TEST(Provider, MultipleLoggerProviders) ASSERT_NE(logs_api::Provider::GetLoggerProvider(), tf); } + +} // namespace From a1699c07f7af3f9bbc401fae5318cbfb5f1121e6 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 03:00:45 +0800 Subject: [PATCH 2/4] Add CHANGELOG entry for #4302 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55405b8d55..00da82a6bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ Increment the: ## [Unreleased] +* [CODE HEALTH] Move SDK common and logs test helpers into anonymous + namespaces + [#4302](https://github.com/open-telemetry/opentelemetry-cpp/pull/4302) + * [CODE HEALTH] Move metrics storage test fixtures into anonymous namespace [#4286](https://github.com/open-telemetry/opentelemetry-cpp/pull/4286) From b481617327b1dcbcdbf90708ab6c782433629f2b Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 14:22:59 +0800 Subject: [PATCH 3/4] Make the lifetime test actually detect a reversed destruction order Review of #4302 pointed out the checker's assertion could pass for the wrong reason. The predicate was `handle && custom_handler_destroyed`, which only aborts when the handler destructor has already run yet a handle is still reachable. If the checker were destroyed first, the case the test exists to catch, custom_handler_destroyed is still false and the whole predicate is false, so it would not abort. It is now `!custom_handler_destroyed || handle`: abort if the checker outruns the handler destructor, or if the handler is still reachable. This is a pre-existing weakness rather than something the namespace move introduced, but the PR described the test as failing loudly on a reversed order, so the test should actually do that. The correct order still passes: the handler is destroyed first and the checker's check passes. Part of #4196 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../common/global_log_handle_singleton_lifetime_test.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sdk/test/common/global_log_handle_singleton_lifetime_test.cc b/sdk/test/common/global_log_handle_singleton_lifetime_test.cc index a09510416d..50c31adc26 100644 --- a/sdk/test/common/global_log_handle_singleton_lifetime_test.cc +++ b/sdk/test/common/global_log_handle_singleton_lifetime_test.cc @@ -22,9 +22,13 @@ class GlobalLogHandlerChecker { using opentelemetry::sdk::common::internal_log::GlobalLogHandler; auto handle = GlobalLogHandler::GetLogHandler(); - if (handle && custom_handler_destroyed) + // The checker must outlive the global handler. Fail if it is destroyed first + // (custom_handler_destroyed still false) or if the handler is somehow still reachable + // (handle non-null). The old "handle && custom_handler_destroyed" only caught the second + // case, so a reversed order slipped through. + if (!custom_handler_destroyed || handle) { - OTEL_INTERNAL_LOG_ERROR("GlobalLogHandler should be destroyed here"); + OTEL_INTERNAL_LOG_ERROR("GlobalLogHandler must be destroyed before the checker"); abort(); } std::cout << "GlobalLogHandlerChecker destroyed and check pass.\n"; From ca2de05e3d6b84c252c65794065a8aa5b2c6624d Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:34:26 +0800 Subject: [PATCH 4/4] Trim code history from the lifetime-checker comment Per review, a code comment should describe the code, not what changed. Drop the sentence comparing against the old predicate; the rationale for the change lives in the PR. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- sdk/test/common/global_log_handle_singleton_lifetime_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sdk/test/common/global_log_handle_singleton_lifetime_test.cc b/sdk/test/common/global_log_handle_singleton_lifetime_test.cc index 50c31adc26..b5f5e64daf 100644 --- a/sdk/test/common/global_log_handle_singleton_lifetime_test.cc +++ b/sdk/test/common/global_log_handle_singleton_lifetime_test.cc @@ -24,8 +24,7 @@ class GlobalLogHandlerChecker auto handle = GlobalLogHandler::GetLogHandler(); // The checker must outlive the global handler. Fail if it is destroyed first // (custom_handler_destroyed still false) or if the handler is somehow still reachable - // (handle non-null). The old "handle && custom_handler_destroyed" only caught the second - // case, so a reversed order slipped through. + // (handle non-null). if (!custom_handler_destroyed || handle) { OTEL_INTERNAL_LOG_ERROR("GlobalLogHandler must be destroyed before the checker");