From a2c48c68de8bd7f0baa2e1f32cd57c056f64d27c Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 29 Oct 2025 15:31:42 -0700 Subject: [PATCH 01/20] Add tag to AggregationConfig for type checking --- .../metrics/aggregation/aggregation_config.h | 14 +++++++++ .../opentelemetry/sdk/metrics/view/view.h | 31 ++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index e64ce09c9d..30526b6778 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -13,6 +13,14 @@ namespace sdk { namespace metrics { + +enum class AggregationConfigType +{ + kDefault, + kHistogram, + kBase2ExponentialHistogram +}; + class AggregationConfig { public: @@ -20,6 +28,8 @@ class AggregationConfig : cardinality_limit_(cardinality_limit) {} + virtual AggregationConfigType GetType() const { return AggregationConfigType::kDefault; } + static const AggregationConfig *GetOrDefault(const AggregationConfig *config) { if (config) @@ -41,6 +51,8 @@ class HistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} + AggregationConfigType GetType() const override { return AggregationConfigType::kHistogram; } + std::vector boundaries_; bool record_min_max_ = true; }; @@ -53,6 +65,8 @@ class Base2ExponentialHistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} + AggregationConfigType GetType() const override { return AggregationConfigType::kBase2ExponentialHistogram; } + size_t max_buckets_ = 160; int32_t max_scale_ = 20; bool record_min_max_ = true; diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index 49c52ff5d3..ebdcc689ef 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" @@ -36,7 +37,35 @@ class View aggregation_type_{aggregation_type}, aggregation_config_{aggregation_config}, attributes_processor_{std::move(attributes_processor)} - {} + { + // Validate that aggregation_type and aggregation_config match + if (aggregation_config_) + { + AggregationConfigType config_type = aggregation_config_->GetType(); + bool valid = false; + + switch (aggregation_type_) + { + case AggregationType::kHistogram: + valid = (config_type == AggregationConfigType::kHistogram); + break; + case AggregationType::kBase2ExponentialHistogram: + valid = (config_type == AggregationConfigType::kBase2ExponentialHistogram); + break; + case AggregationType::kDefault: + case AggregationType::kDrop: + case AggregationType::kLastValue: + case AggregationType::kSum: + valid = (config_type == AggregationConfigType::kDefault); + break; + } + + if (!valid) + { + throw std::invalid_argument("AggregationType and AggregationConfig type mismatch"); + } + } + } virtual ~View() = default; From d2330d1e5dbb2ce61eafedf5a0d16f0cdb692365 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 30 Oct 2025 17:39:11 -0700 Subject: [PATCH 02/20] Add test --- .../opentelemetry/sdk/metrics/view/view.h | 4 +- sdk/test/metrics/view_registry_test.cc | 66 +++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index ebdcc689ef..1c0879c2f9 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -43,7 +43,7 @@ class View { AggregationConfigType config_type = aggregation_config_->GetType(); bool valid = false; - + switch (aggregation_type_) { case AggregationType::kHistogram: @@ -59,7 +59,7 @@ class View valid = (config_type == AggregationConfigType::kDefault); break; } - + if (!valid) { throw std::invalid_argument("AggregationType and AggregationConfig type mismatch"); diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 600b843f7d..4f7fada7bb 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -10,6 +10,7 @@ #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/nostd/unique_ptr.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" +#include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/view/instrument_selector.h" #include "opentelemetry/sdk/metrics/view/meter_selector.h" @@ -93,3 +94,68 @@ TEST(ViewRegistry, FindNonExistingView) EXPECT_EQ(status, true); #endif } + +// Tests for View class AggregationType and AggregationConfig consistency validation + +TEST(View, ValidHistogramConfigWithHistogramType) +{ + // Valid: Histogram aggregation type with HistogramAggregationConfig + auto histogram_config = std::make_shared(); + histogram_config->boundaries_ = {0.0, 10.0, 100.0}; + + EXPECT_NO_THROW({ + View view("test_histogram", "description", AggregationType::kHistogram, histogram_config); + }); +} + +TEST(View, InvalidHistogramConfigWithSumType) +{ + // Invalid: Sum aggregation type with HistogramAggregationConfig + auto histogram_config = std::make_shared(); + histogram_config->boundaries_ = {0.0, 10.0, 100.0}; + + EXPECT_THROW( + { + View view("test_sum", "description", AggregationType::kSum, histogram_config); + }, + std::invalid_argument + ); +} + +TEST(View, InvalidHistogramConfigWithDefaultType) +{ + // Invalid: Default aggregation type with HistogramAggregationConfig + auto histogram_config = std::make_shared(); + + EXPECT_THROW( + { + View view("test_default", "description", AggregationType::kDefault, histogram_config); + }, + std::invalid_argument + ); +} + +TEST(View, ValidNullConfig) +{ + // Valid: Null config should work with any aggregation type + EXPECT_NO_THROW({ + View view("test_null_config", "description", AggregationType::kHistogram, nullptr); + }); + + EXPECT_NO_THROW({ + View view("test_null_config2", "description", AggregationType::kSum, nullptr); + }); +} + +TEST(View, InvalidDefaultConfigWithHistogramType) +{ + // Invalid: Histogram aggregation type with default AggregationConfig (not HistogramAggregationConfig) + auto default_config = std::make_shared(); + + EXPECT_THROW( + { + View view("test_histogram", "description", AggregationType::kHistogram, default_config); + }, + std::invalid_argument + ); +} From 177d85a5f6937bb88beb494d2f51892cb1300975 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 30 Oct 2025 20:30:34 -0700 Subject: [PATCH 03/20] Fix format --- .../metrics/aggregation/aggregation_config.h | 5 ++- .../opentelemetry/sdk/metrics/view/view.h | 2 +- sdk/test/metrics/view_registry_test.cc | 38 +++++++------------ 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index 30526b6778..9ac552ada9 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -65,7 +65,10 @@ class Base2ExponentialHistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} - AggregationConfigType GetType() const override { return AggregationConfigType::kBase2ExponentialHistogram; } + AggregationConfigType GetType() const override + { + return AggregationConfigType::kBase2ExponentialHistogram; + } size_t max_buckets_ = 160; int32_t max_scale_ = 20; diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index 1c0879c2f9..70909eba56 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -42,7 +42,7 @@ class View if (aggregation_config_) { AggregationConfigType config_type = aggregation_config_->GetType(); - bool valid = false; + bool valid = false; switch (aggregation_type_) { diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 4f7fada7bb..f21bb022ff 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -100,7 +100,7 @@ TEST(ViewRegistry, FindNonExistingView) TEST(View, ValidHistogramConfigWithHistogramType) { // Valid: Histogram aggregation type with HistogramAggregationConfig - auto histogram_config = std::make_shared(); + auto histogram_config = std::make_shared(); histogram_config->boundaries_ = {0.0, 10.0, 100.0}; EXPECT_NO_THROW({ @@ -111,15 +111,12 @@ TEST(View, ValidHistogramConfigWithHistogramType) TEST(View, InvalidHistogramConfigWithSumType) { // Invalid: Sum aggregation type with HistogramAggregationConfig - auto histogram_config = std::make_shared(); + auto histogram_config = std::make_shared(); histogram_config->boundaries_ = {0.0, 10.0, 100.0}; EXPECT_THROW( - { - View view("test_sum", "description", AggregationType::kSum, histogram_config); - }, - std::invalid_argument - ); + { View view("test_sum", "description", AggregationType::kSum, histogram_config); }, + std::invalid_argument); } TEST(View, InvalidHistogramConfigWithDefaultType) @@ -128,34 +125,27 @@ TEST(View, InvalidHistogramConfigWithDefaultType) auto histogram_config = std::make_shared(); EXPECT_THROW( - { - View view("test_default", "description", AggregationType::kDefault, histogram_config); - }, - std::invalid_argument - ); + { View view("test_default", "description", AggregationType::kDefault, histogram_config); }, + std::invalid_argument); } TEST(View, ValidNullConfig) { // Valid: Null config should work with any aggregation type - EXPECT_NO_THROW({ - View view("test_null_config", "description", AggregationType::kHistogram, nullptr); - }); + EXPECT_NO_THROW( + { View view("test_null_config", "description", AggregationType::kHistogram, nullptr); }); - EXPECT_NO_THROW({ - View view("test_null_config2", "description", AggregationType::kSum, nullptr); - }); + EXPECT_NO_THROW( + { View view("test_null_config2", "description", AggregationType::kSum, nullptr); }); } TEST(View, InvalidDefaultConfigWithHistogramType) { - // Invalid: Histogram aggregation type with default AggregationConfig (not HistogramAggregationConfig) + // Invalid: Histogram aggregation type with default AggregationConfig (not + // HistogramAggregationConfig) auto default_config = std::make_shared(); EXPECT_THROW( - { - View view("test_histogram", "description", AggregationType::kHistogram, default_config); - }, - std::invalid_argument - ); + { View view("test_histogram", "description", AggregationType::kHistogram, default_config); }, + std::invalid_argument); } From db221d80edee85f1f2a2504dc433bee71bac151c Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 30 Oct 2025 20:35:54 -0700 Subject: [PATCH 04/20] Address feedback --- .../metrics/aggregation/aggregation_config.h | 17 ++++------------- .../opentelemetry/sdk/metrics/view/view.h | 10 +++++----- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index 9ac552ada9..d205c0b082 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -5,6 +5,7 @@ #include +#include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/version.h" @@ -14,13 +15,6 @@ namespace sdk namespace metrics { -enum class AggregationConfigType -{ - kDefault, - kHistogram, - kBase2ExponentialHistogram -}; - class AggregationConfig { public: @@ -28,7 +22,7 @@ class AggregationConfig : cardinality_limit_(cardinality_limit) {} - virtual AggregationConfigType GetType() const { return AggregationConfigType::kDefault; } + virtual AggregationType GetType() const { return AggregationType::kDefault; } static const AggregationConfig *GetOrDefault(const AggregationConfig *config) { @@ -51,7 +45,7 @@ class HistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} - AggregationConfigType GetType() const override { return AggregationConfigType::kHistogram; } + AggregationType GetType() const override { return AggregationType::kHistogram; } std::vector boundaries_; bool record_min_max_ = true; @@ -65,10 +59,7 @@ class Base2ExponentialHistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} - AggregationConfigType GetType() const override - { - return AggregationConfigType::kBase2ExponentialHistogram; - } + AggregationType GetType() const override { return AggregationType::kBase2ExponentialHistogram; } size_t max_buckets_ = 160; int32_t max_scale_ = 20; diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index 70909eba56..68671db176 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -41,22 +41,22 @@ class View // Validate that aggregation_type and aggregation_config match if (aggregation_config_) { - AggregationConfigType config_type = aggregation_config_->GetType(); - bool valid = false; + AggregationType config_type = aggregation_config_->GetType(); + bool valid = false; switch (aggregation_type_) { case AggregationType::kHistogram: - valid = (config_type == AggregationConfigType::kHistogram); + valid = (config_type == AggregationType::kHistogram); break; case AggregationType::kBase2ExponentialHistogram: - valid = (config_type == AggregationConfigType::kBase2ExponentialHistogram); + valid = (config_type == AggregationType::kBase2ExponentialHistogram); break; case AggregationType::kDefault: case AggregationType::kDrop: case AggregationType::kLastValue: case AggregationType::kSum: - valid = (config_type == AggregationConfigType::kDefault); + valid = (config_type == AggregationType::kDefault); break; } From 5dfc194ef355536708b6969f83c68cb2287f4412 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 3 Nov 2025 11:39:00 -0800 Subject: [PATCH 05/20] Support build without exception --- .../sdk/metrics/aggregation/aggregation_config.h | 9 ++++++--- sdk/include/opentelemetry/sdk/metrics/view/view.h | 5 +++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index d205c0b082..088a115f9e 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -22,7 +22,7 @@ class AggregationConfig : cardinality_limit_(cardinality_limit) {} - virtual AggregationType GetType() const { return AggregationType::kDefault; } + virtual AggregationType GetType() const noexcept { return AggregationType::kDefault; } static const AggregationConfig *GetOrDefault(const AggregationConfig *config) { @@ -45,7 +45,7 @@ class HistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} - AggregationType GetType() const override { return AggregationType::kHistogram; } + AggregationType GetType() const noexcept override { return AggregationType::kHistogram; } std::vector boundaries_; bool record_min_max_ = true; @@ -59,7 +59,10 @@ class Base2ExponentialHistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} - AggregationType GetType() const override { return AggregationType::kBase2ExponentialHistogram; } + AggregationType GetType() const noexcept override + { + return AggregationType::kBase2ExponentialHistogram; + } size_t max_buckets_ = 160; int32_t max_scale_ = 20; diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index 68671db176..62e9b72436 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -62,7 +62,12 @@ class View if (!valid) { +#if defined(__cpp_exceptions) throw std::invalid_argument("AggregationType and AggregationConfig type mismatch"); +#else + std::cerr << "AggregationType and AggregationConfig type mismatch" << std::endl; + std::abort(); +#endif } } } From 2c4266b1c0be8d0ff3b9f0c50de5b458cb833bac Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 3 Nov 2025 11:47:48 -0800 Subject: [PATCH 06/20] Remove logging --- sdk/include/opentelemetry/sdk/metrics/view/view.h | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index 62e9b72436..a46a954154 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -65,7 +65,6 @@ class View #if defined(__cpp_exceptions) throw std::invalid_argument("AggregationType and AggregationConfig type mismatch"); #else - std::cerr << "AggregationType and AggregationConfig type mismatch" << std::endl; std::abort(); #endif } From 6f04f065a2eb8d6d6114a048be21634e7512e0e4 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 3 Nov 2025 15:59:58 -0800 Subject: [PATCH 07/20] Handle unittest in noexception build --- sdk/test/metrics/view_registry_test.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index f21bb022ff..8308c9027e 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -114,9 +114,14 @@ TEST(View, InvalidHistogramConfigWithSumType) auto histogram_config = std::make_shared(); histogram_config->boundaries_ = {0.0, 10.0, 100.0}; +#if defined(__cpp_exceptions) EXPECT_THROW( { View view("test_sum", "description", AggregationType::kSum, histogram_config); }, std::invalid_argument); +#else + EXPECT_DEATH( + { View view("test_sum", "description", AggregationType::kSum, histogram_config); }, ""); +#endif } TEST(View, InvalidHistogramConfigWithDefaultType) @@ -124,9 +129,15 @@ TEST(View, InvalidHistogramConfigWithDefaultType) // Invalid: Default aggregation type with HistogramAggregationConfig auto histogram_config = std::make_shared(); +#if defined(__cpp_exceptions) EXPECT_THROW( { View view("test_default", "description", AggregationType::kDefault, histogram_config); }, std::invalid_argument); +#else + EXPECT_DEATH( + { View view("test_default", "description", AggregationType::kDefault, histogram_config); }, + ""); +#endif } TEST(View, ValidNullConfig) @@ -145,7 +156,13 @@ TEST(View, InvalidDefaultConfigWithHistogramType) // HistogramAggregationConfig) auto default_config = std::make_shared(); +#if defined(__cpp_exceptions) EXPECT_THROW( { View view("test_histogram", "description", AggregationType::kHistogram, default_config); }, std::invalid_argument); +#else + EXPECT_DEATH( + { View view("test_histogram", "description", AggregationType::kHistogram, default_config); }, + ""); +#endif } From 363eb4605d786097a209e001807fa3879498aa59 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 3 Nov 2025 16:37:14 -0800 Subject: [PATCH 08/20] Fix EXPECT_NO_THROW for no-exception build --- sdk/test/metrics/view_registry_test.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 8308c9027e..8639e50320 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -103,9 +103,13 @@ TEST(View, ValidHistogramConfigWithHistogramType) auto histogram_config = std::make_shared(); histogram_config->boundaries_ = {0.0, 10.0, 100.0}; +#if defined(__cpp_exceptions) EXPECT_NO_THROW({ View view("test_histogram", "description", AggregationType::kHistogram, histogram_config); }); +#else + View view("test_histogram", "description", AggregationType::kHistogram, histogram_config); +#endif } TEST(View, InvalidHistogramConfigWithSumType) @@ -143,11 +147,16 @@ TEST(View, InvalidHistogramConfigWithDefaultType) TEST(View, ValidNullConfig) { // Valid: Null config should work with any aggregation type +#if defined(__cpp_exceptions) EXPECT_NO_THROW( { View view("test_null_config", "description", AggregationType::kHistogram, nullptr); }); EXPECT_NO_THROW( { View view("test_null_config2", "description", AggregationType::kSum, nullptr); }); +#else + View view("test_null_config", "description", AggregationType::kHistogram, nullptr); + View view2("test_null_config2", "description", AggregationType::kSum, nullptr); +#endif } TEST(View, InvalidDefaultConfigWithHistogramType) From b500f613ad739194503b577382863b7b746b6b1c Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 3 Nov 2025 17:04:30 -0800 Subject: [PATCH 09/20] Fix iwyu --- sdk/test/metrics/view_registry_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 8639e50320..b3d7805411 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -2,15 +2,17 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include #include +#include #include "opentelemetry/common/macros.h" #include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/nostd/unique_ptr.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" -#include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/view/instrument_selector.h" #include "opentelemetry/sdk/metrics/view/meter_selector.h" From af73f3a6aefee60b71b4f23d5ade2f67e6b5964d Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Tue, 4 Nov 2025 17:01:38 -0800 Subject: [PATCH 10/20] Move check to AddView --- .../opentelemetry/sdk/metrics/view/view.h | 34 +-------- .../sdk/metrics/view/view_registry.h | 51 +++++++++++++ sdk/test/metrics/view_registry_test.cc | 73 +++++++------------ 3 files changed, 79 insertions(+), 79 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index a46a954154..882c31cc37 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -37,39 +37,7 @@ class View aggregation_type_{aggregation_type}, aggregation_config_{aggregation_config}, attributes_processor_{std::move(attributes_processor)} - { - // Validate that aggregation_type and aggregation_config match - if (aggregation_config_) - { - AggregationType config_type = aggregation_config_->GetType(); - bool valid = false; - - switch (aggregation_type_) - { - case AggregationType::kHistogram: - valid = (config_type == AggregationType::kHistogram); - break; - case AggregationType::kBase2ExponentialHistogram: - valid = (config_type == AggregationType::kBase2ExponentialHistogram); - break; - case AggregationType::kDefault: - case AggregationType::kDrop: - case AggregationType::kLastValue: - case AggregationType::kSum: - valid = (config_type == AggregationType::kDefault); - break; - } - - if (!valid) - { -#if defined(__cpp_exceptions) - throw std::invalid_argument("AggregationType and AggregationConfig type mismatch"); -#else - std::abort(); -#endif - } - } - } + {} virtual ~View() = default; diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index fd1e4bee0c..ba9649ac5c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -5,12 +5,14 @@ #include #include +#include #include #include #include #include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" +#include "opentelemetry/sdk/metrics/aggregation/default_aggregation.h" #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/view/instrument_selector.h" #include "opentelemetry/sdk/metrics/view/meter_selector.h" @@ -47,6 +49,55 @@ class ViewRegistry { // TBD - thread-safe ? + // Validate parameters + if (!instrument_selector || !meter_selector || !view) + { +#if defined(__cpp_exceptions) + throw std::invalid_argument("instrument_selector, meter_selector, and view cannot be null"); +#else + std::abort(); +#endif + } + + auto aggregation_config = view->GetAggregationConfig(); + if (aggregation_config) + { + bool valid = false; + auto aggregation_config_type = aggregation_config->GetType(); + auto aggregation_type = view->GetAggregationType(); + + if (aggregation_type == AggregationType::kDefault) + { + bool is_monotonic; + aggregation_type = DefaultAggregation::GetDefaultAggregationType( + instrument_selector->GetInstrumentType(), is_monotonic); + } + + switch (aggregation_type) + { + case AggregationType::kHistogram: + valid = (aggregation_config_type == AggregationType::kHistogram); + break; + case AggregationType::kBase2ExponentialHistogram: + valid = (aggregation_config_type == AggregationType::kBase2ExponentialHistogram); + break; + case AggregationType::kDrop: + case AggregationType::kLastValue: + case AggregationType::kSum: + valid = (aggregation_config_type == AggregationType::kDefault); + break; + } + + if (!valid) + { +#if defined(__cpp_exceptions) + throw std::invalid_argument("AggregationType and AggregationConfig type mismatch"); +#else + std::abort(); +#endif + } + } + auto registered_view = std::unique_ptr(new RegisteredView{ std::move(instrument_selector), std::move(meter_selector), std::move(view)}); registered_views_.push_back(std::move(registered_view)); diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index b3d7805411..b25a9bfd7c 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -97,83 +97,64 @@ TEST(ViewRegistry, FindNonExistingView) #endif } -// Tests for View class AggregationType and AggregationConfig consistency validation +// Tests for ViewRegistry::AddView null parameter validation -TEST(View, ValidHistogramConfigWithHistogramType) +TEST(ViewRegistry, AddViewWithNullInstrumentSelector) { - // Valid: Histogram aggregation type with HistogramAggregationConfig - auto histogram_config = std::make_shared(); - histogram_config->boundaries_ = {0.0, 10.0, 100.0}; + ViewRegistry registry; + std::unique_ptr meter_selector{new MeterSelector("name", "version", "schema")}; + std::unique_ptr view{new View("test_view")}; #if defined(__cpp_exceptions) - EXPECT_NO_THROW({ - View view("test_histogram", "description", AggregationType::kHistogram, histogram_config); - }); + EXPECT_THROW( + { registry.AddView(nullptr, std::move(meter_selector), std::move(view)); }, + std::invalid_argument); #else - View view("test_histogram", "description", AggregationType::kHistogram, histogram_config); + EXPECT_DEATH({ registry.AddView(nullptr, std::move(meter_selector), std::move(view)); }, ""); #endif } -TEST(View, InvalidHistogramConfigWithSumType) +TEST(ViewRegistry, AddViewWithNullMeterSelector) { - // Invalid: Sum aggregation type with HistogramAggregationConfig - auto histogram_config = std::make_shared(); - histogram_config->boundaries_ = {0.0, 10.0, 100.0}; + ViewRegistry registry; + std::unique_ptr instrument_selector{ + new InstrumentSelector(InstrumentType::kCounter, "instrument_name", "unit")}; + std::unique_ptr view{new View("test_view")}; #if defined(__cpp_exceptions) EXPECT_THROW( - { View view("test_sum", "description", AggregationType::kSum, histogram_config); }, + { registry.AddView(std::move(instrument_selector), nullptr, std::move(view)); }, std::invalid_argument); #else - EXPECT_DEATH( - { View view("test_sum", "description", AggregationType::kSum, histogram_config); }, ""); + EXPECT_DEATH({ registry.AddView(std::move(instrument_selector), nullptr, std::move(view)); }, ""); #endif } -TEST(View, InvalidHistogramConfigWithDefaultType) +TEST(ViewRegistry, AddViewWithNullView) { - // Invalid: Default aggregation type with HistogramAggregationConfig - auto histogram_config = std::make_shared(); + ViewRegistry registry; + std::unique_ptr instrument_selector{ + new InstrumentSelector(InstrumentType::kCounter, "instrument_name", "unit")}; + std::unique_ptr meter_selector{new MeterSelector("name", "version", "schema")}; #if defined(__cpp_exceptions) EXPECT_THROW( - { View view("test_default", "description", AggregationType::kDefault, histogram_config); }, + { registry.AddView(std::move(instrument_selector), std::move(meter_selector), nullptr); }, std::invalid_argument); #else EXPECT_DEATH( - { View view("test_default", "description", AggregationType::kDefault, histogram_config); }, + { registry.AddView(std::move(instrument_selector), std::move(meter_selector), nullptr); }, ""); #endif } -TEST(View, ValidNullConfig) -{ - // Valid: Null config should work with any aggregation type -#if defined(__cpp_exceptions) - EXPECT_NO_THROW( - { View view("test_null_config", "description", AggregationType::kHistogram, nullptr); }); - - EXPECT_NO_THROW( - { View view("test_null_config2", "description", AggregationType::kSum, nullptr); }); -#else - View view("test_null_config", "description", AggregationType::kHistogram, nullptr); - View view2("test_null_config2", "description", AggregationType::kSum, nullptr); -#endif -} - -TEST(View, InvalidDefaultConfigWithHistogramType) +TEST(ViewRegistry, AddViewWithAllNullParameters) { - // Invalid: Histogram aggregation type with default AggregationConfig (not - // HistogramAggregationConfig) - auto default_config = std::make_shared(); + ViewRegistry registry; #if defined(__cpp_exceptions) - EXPECT_THROW( - { View view("test_histogram", "description", AggregationType::kHistogram, default_config); }, - std::invalid_argument); + EXPECT_THROW({ registry.AddView(nullptr, nullptr, nullptr); }, std::invalid_argument); #else - EXPECT_DEATH( - { View view("test_histogram", "description", AggregationType::kHistogram, default_config); }, - ""); + EXPECT_DEATH({ registry.AddView(nullptr, nullptr, nullptr); }, ""); #endif } From 6179d6fa93790c84de3e2a7ea9912d2a0a5748d7 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Tue, 4 Nov 2025 17:10:40 -0800 Subject: [PATCH 11/20] Handle kDefault in AddView --- sdk/include/opentelemetry/sdk/metrics/view/view_registry.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index ba9649ac5c..6dc95b07ff 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -78,14 +78,20 @@ class ViewRegistry case AggregationType::kHistogram: valid = (aggregation_config_type == AggregationType::kHistogram); break; + case AggregationType::kBase2ExponentialHistogram: valid = (aggregation_config_type == AggregationType::kBase2ExponentialHistogram); break; + case AggregationType::kDrop: case AggregationType::kLastValue: case AggregationType::kSum: valid = (aggregation_config_type == AggregationType::kDefault); break; + + default: + // Unreachable: all AggregationType enum values are handled above + std::abort(); } if (!valid) From d3b4c42ba0a1433eb28ce40169b5b80b2b1f3856 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Tue, 4 Nov 2025 17:29:33 -0800 Subject: [PATCH 12/20] Fix meter provider test --- sdk/test/metrics/meter_provider_sdk_test.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sdk/test/metrics/meter_provider_sdk_test.cc b/sdk/test/metrics/meter_provider_sdk_test.cc index 0c1f4d7de1..93d996f204 100644 --- a/sdk/test/metrics/meter_provider_sdk_test.cc +++ b/sdk/test/metrics/meter_provider_sdk_test.cc @@ -36,8 +36,6 @@ using namespace opentelemetry::sdk::metrics; TEST(MeterProvider, GetMeter) { MeterProvider mp1; - // std::unique_ptr view{std::unique_ptr()}; - // MeterProvider mp1(std::move(exporters), std::move(readers), std::move(views); auto m1 = mp1.GetMeter("test"); auto m2 = mp1.GetMeter("test"); auto m3 = mp1.GetMeter("different", "1.0.0"); @@ -66,7 +64,7 @@ TEST(MeterProvider, GetMeter) std::unique_ptr reader{new MockMetricReader(std::move(exporter))}; mp1.AddMetricReader(std::move(reader)); - std::unique_ptr view{std::unique_ptr()}; + std::unique_ptr view{new View("test_view")}; std::unique_ptr instrument_selector{ new InstrumentSelector(InstrumentType::kCounter, "instru1", "unit1")}; std::unique_ptr meter_selector{new MeterSelector("name1", "version1", "schema1")}; From 9dd5eaeadfa6345b07d572855680a6cf32c4f928 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 5 Nov 2025 11:08:48 -0800 Subject: [PATCH 13/20] Fix iwyu --- sdk/test/metrics/view_registry_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index b25a9bfd7c..3a1b06a344 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -5,14 +5,12 @@ #include #include #include -#include #include "opentelemetry/common/macros.h" #include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/nostd/unique_ptr.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" -#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/view/instrument_selector.h" #include "opentelemetry/sdk/metrics/view/meter_selector.h" From 36ef49a230727b6acee2bab30280aae6228f2139 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 5 Nov 2025 13:44:56 -0800 Subject: [PATCH 14/20] Remove noexcept on AddView --- sdk/include/opentelemetry/sdk/metrics/meter_context.h | 2 +- sdk/src/metrics/meter_context.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/meter_context.h b/sdk/include/opentelemetry/sdk/metrics/meter_context.h index 53f6298049..b20c68a75f 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter_context.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter_context.h @@ -136,7 +136,7 @@ class MeterContext : public std::enable_shared_from_this */ void AddView(std::unique_ptr instrument_selector, std::unique_ptr meter_selector, - std::unique_ptr view) noexcept; + std::unique_ptr view); #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW diff --git a/sdk/src/metrics/meter_context.cc b/sdk/src/metrics/meter_context.cc index 863ef21839..892430f394 100644 --- a/sdk/src/metrics/meter_context.cc +++ b/sdk/src/metrics/meter_context.cc @@ -106,7 +106,7 @@ void MeterContext::AddMetricReader(std::shared_ptr reader, void MeterContext::AddView(std::unique_ptr instrument_selector, std::unique_ptr meter_selector, - std::unique_ptr view) noexcept + std::unique_ptr view) { views_->AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); } From 5e96531b5b685ceeedef282200e357d943edb236 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 5 Nov 2025 17:50:30 -0800 Subject: [PATCH 15/20] Add changelog --- CHANGELOG.md | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 71e358a95a..c1cff07925 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,21 +15,25 @@ Increment the: ## [Unreleased] +* [METRICS] Add tag to AggregationConfig for aggregation type validation + [#3732](https://github.com/open-telemetry/opentelemetry-cpp/pull/3732) + * [TEST] Remove workaround for metrics cardinality limit test [#3663](https://github.com/open-telemetry/opentelemetry-cpp/pull/3663) -* [METRICS] Allow registering one callback for multiple instruments - [#3667](https://github.com/open-telemetry/opentelemetry-cpp/pull/3667) * [SDK] Fix typo in hashmap method GetEnteries [#3680](https://github.com/open-telemetry/opentelemetry-cpp/pull/3680) -* [CI] Upgrade tools/vcpkg to 2025.09.17 - [#3701](https://github.com/open-telemetry/opentelemetry-cpp/pull/3701) +## [1.23 2025-09-25] -* [CONFIGURATION] File configuration - prometheus translation - [#3715](https://github.com/open-telemetry/opentelemetry-cpp/pull/3715) +* [CodeHealth] Fix clang-tidy warnings part 6 + [#3507](https://github.com/open-telemetry/opentelemetry-cpp/pull/3507) -* [BUILD] Upgrade to opentelemetry-proto 1.8.0 +* [CMAKE] Add CMake scripts to find or fetch curl and find zlib + [#3526](https://github.com/open-telemetry/opentelemetry-cpp/pull/3526) + +* [REMOVAL] remove unused ci bash scripts + [#3541](https://github.com/open-telemetry/opentelemetry-cpp/pull/354* [BUILD] Upgrade to opentelemetry-proto 1.8.0 [#3730](https://github.com/open-telemetry/opentelemetry-cpp/pull/3730) * [CONFIGURATION] File configuration - tls @@ -47,16 +51,7 @@ New Features: file, instead of using environment variables. * See [opentelemetry-configuration](https://github.com/open-telemetry/opentelemetry-configuration) -## [1.23 2025-09-25] - -* [CodeHealth] Fix clang-tidy warnings part 6 - [#3507](https://github.com/open-telemetry/opentelemetry-cpp/pull/3507) - -* [CMAKE] Add CMake scripts to find or fetch curl and find zlib - [#3526](https://github.com/open-telemetry/opentelemetry-cpp/pull/3526) - -* [REMOVAL] remove unused ci bash scripts - [#3541](https://github.com/open-telemetry/opentelemetry-cpp/pull/3541) +1) * Bump step-security/harden-runner from 2.12.2 to 2.13.0 [#3542](https://github.com/open-telemetry/opentelemetry-cpp/pull/3542) From f30cea626c13948c0c46eee551ea32c1cd1f3f46 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 6 Nov 2025 07:23:21 -0800 Subject: [PATCH 16/20] Update changelog --- CHANGELOG.md | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1cff07925..317c4f440f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,19 +21,19 @@ Increment the: * [TEST] Remove workaround for metrics cardinality limit test [#3663](https://github.com/open-telemetry/opentelemetry-cpp/pull/3663) +* [METRICS] Allow registering one callback for multiple instruments + [#3667](https://github.com/open-telemetry/opentelemetry-cpp/pull/3667) + * [SDK] Fix typo in hashmap method GetEnteries [#3680](https://github.com/open-telemetry/opentelemetry-cpp/pull/3680) -## [1.23 2025-09-25] +* [CI] Upgrade tools/vcpkg to 2025.09.17 + [#3701](https://github.com/open-telemetry/opentelemetry-cpp/pull/3701) -* [CodeHealth] Fix clang-tidy warnings part 6 - [#3507](https://github.com/open-telemetry/opentelemetry-cpp/pull/3507) +* [CONFIGURATION] File configuration - prometheus translation + [#3715](https://github.com/open-telemetry/opentelemetry-cpp/pull/3715) -* [CMAKE] Add CMake scripts to find or fetch curl and find zlib - [#3526](https://github.com/open-telemetry/opentelemetry-cpp/pull/3526) - -* [REMOVAL] remove unused ci bash scripts - [#3541](https://github.com/open-telemetry/opentelemetry-cpp/pull/354* [BUILD] Upgrade to opentelemetry-proto 1.8.0 +* [BUILD] Upgrade to opentelemetry-proto 1.8.0 [#3730](https://github.com/open-telemetry/opentelemetry-cpp/pull/3730) * [CONFIGURATION] File configuration - tls @@ -51,7 +51,16 @@ New Features: file, instead of using environment variables. * See [opentelemetry-configuration](https://github.com/open-telemetry/opentelemetry-configuration) -1) +## [1.23 2025-09-25] + +* [CodeHealth] Fix clang-tidy warnings part 6 + [#3507](https://github.com/open-telemetry/opentelemetry-cpp/pull/3507) + +* [CMAKE] Add CMake scripts to find or fetch curl and find zlib + [#3526](https://github.com/open-telemetry/opentelemetry-cpp/pull/3526) + +* [REMOVAL] remove unused ci bash scripts + [#3541](https://github.com/open-telemetry/opentelemetry-cpp/pull/3541) * Bump step-security/harden-runner from 2.12.2 to 2.13.0 [#3542](https://github.com/open-telemetry/opentelemetry-cpp/pull/3542) From 01889b25e309248108a8b5195aaa1b853efa44fc Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 10 Nov 2025 10:45:39 -0800 Subject: [PATCH 17/20] Log View validation error and ignore it --- .../sdk/metrics/view/view_registry.h | 20 +++++----- sdk/test/metrics/view_registry_test.cc | 38 +++++-------------- 2 files changed, 18 insertions(+), 40 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index 6dc95b07ff..91c65244cc 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -5,12 +5,12 @@ #include #include -#include #include #include #include #include "opentelemetry/nostd/function_ref.h" +#include "opentelemetry/sdk/common/global_log_handler.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" #include "opentelemetry/sdk/metrics/aggregation/default_aggregation.h" #include "opentelemetry/sdk/metrics/instruments.h" @@ -52,11 +52,10 @@ class ViewRegistry // Validate parameters if (!instrument_selector || !meter_selector || !view) { -#if defined(__cpp_exceptions) - throw std::invalid_argument("instrument_selector, meter_selector, and view cannot be null"); -#else - std::abort(); -#endif + OTEL_INTERNAL_LOG_ERROR( + "[ViewRegistry::AddView] Invalid parameters: instrument_selector, meter_selector, and " + "view cannot be null. Ignoring AddView call."); + return; } auto aggregation_config = view->GetAggregationConfig(); @@ -96,11 +95,10 @@ class ViewRegistry if (!valid) { -#if defined(__cpp_exceptions) - throw std::invalid_argument("AggregationType and AggregationConfig type mismatch"); -#else - std::abort(); -#endif + OTEL_INTERNAL_LOG_ERROR( + "[ViewRegistry::AddView] AggregationType and AggregationConfig type mismatch. " + "Ignoring AddView call."); + return; } } diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 3a1b06a344..009c67278f 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -96,6 +95,7 @@ TEST(ViewRegistry, FindNonExistingView) } // Tests for ViewRegistry::AddView null parameter validation +// These should log errors and ignore the call instead of throwing or aborting TEST(ViewRegistry, AddViewWithNullInstrumentSelector) { @@ -103,13 +103,8 @@ TEST(ViewRegistry, AddViewWithNullInstrumentSelector) std::unique_ptr meter_selector{new MeterSelector("name", "version", "schema")}; std::unique_ptr view{new View("test_view")}; -#if defined(__cpp_exceptions) - EXPECT_THROW( - { registry.AddView(nullptr, std::move(meter_selector), std::move(view)); }, - std::invalid_argument); -#else - EXPECT_DEATH({ registry.AddView(nullptr, std::move(meter_selector), std::move(view)); }, ""); -#endif + // Should not throw or abort, just log and ignore + registry.AddView(nullptr, std::move(meter_selector), std::move(view)); } TEST(ViewRegistry, AddViewWithNullMeterSelector) @@ -119,13 +114,8 @@ TEST(ViewRegistry, AddViewWithNullMeterSelector) new InstrumentSelector(InstrumentType::kCounter, "instrument_name", "unit")}; std::unique_ptr view{new View("test_view")}; -#if defined(__cpp_exceptions) - EXPECT_THROW( - { registry.AddView(std::move(instrument_selector), nullptr, std::move(view)); }, - std::invalid_argument); -#else - EXPECT_DEATH({ registry.AddView(std::move(instrument_selector), nullptr, std::move(view)); }, ""); -#endif + // Should not throw or abort, just log and ignore + registry.AddView(std::move(instrument_selector), nullptr, std::move(view)); } TEST(ViewRegistry, AddViewWithNullView) @@ -135,24 +125,14 @@ TEST(ViewRegistry, AddViewWithNullView) new InstrumentSelector(InstrumentType::kCounter, "instrument_name", "unit")}; std::unique_ptr meter_selector{new MeterSelector("name", "version", "schema")}; -#if defined(__cpp_exceptions) - EXPECT_THROW( - { registry.AddView(std::move(instrument_selector), std::move(meter_selector), nullptr); }, - std::invalid_argument); -#else - EXPECT_DEATH( - { registry.AddView(std::move(instrument_selector), std::move(meter_selector), nullptr); }, - ""); -#endif + // Should not throw or abort, just log and ignore + registry.AddView(std::move(instrument_selector), std::move(meter_selector), nullptr); } TEST(ViewRegistry, AddViewWithAllNullParameters) { ViewRegistry registry; -#if defined(__cpp_exceptions) - EXPECT_THROW({ registry.AddView(nullptr, nullptr, nullptr); }, std::invalid_argument); -#else - EXPECT_DEATH({ registry.AddView(nullptr, nullptr, nullptr); }, ""); -#endif + // Should not throw or abort, just log and ignore + registry.AddView(nullptr, nullptr, nullptr); } From 3867d1d68af930f1c443ae59e83f5c306b8c2900 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 10 Nov 2025 11:02:31 -0800 Subject: [PATCH 18/20] Add back noexcept on AddView --- sdk/include/opentelemetry/sdk/metrics/meter_context.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/meter_context.h b/sdk/include/opentelemetry/sdk/metrics/meter_context.h index b20c68a75f..53f6298049 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter_context.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter_context.h @@ -136,7 +136,7 @@ class MeterContext : public std::enable_shared_from_this */ void AddView(std::unique_ptr instrument_selector, std::unique_ptr meter_selector, - std::unique_ptr view); + std::unique_ptr view) noexcept; #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW From c1c5d0ad5fd1e656e9c9a76ac8a59c073021ae5b Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 10 Nov 2025 11:39:38 -0800 Subject: [PATCH 19/20] Add noexcept back to AddView implementation --- sdk/src/metrics/meter_context.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/metrics/meter_context.cc b/sdk/src/metrics/meter_context.cc index 892430f394..863ef21839 100644 --- a/sdk/src/metrics/meter_context.cc +++ b/sdk/src/metrics/meter_context.cc @@ -106,7 +106,7 @@ void MeterContext::AddMetricReader(std::shared_ptr reader, void MeterContext::AddView(std::unique_ptr instrument_selector, std::unique_ptr meter_selector, - std::unique_ptr view) + std::unique_ptr view) noexcept { views_->AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); } From b61e52894962a909cff51956900a35e5d636f23b Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Fri, 14 Nov 2025 13:20:40 -0800 Subject: [PATCH 20/20] Address feedback --- sdk/include/opentelemetry/sdk/metrics/view/view.h | 1 - sdk/include/opentelemetry/sdk/metrics/view/view_registry.h | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index 882c31cc37..49c52ff5d3 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -4,7 +4,6 @@ #pragma once #include -#include #include #include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index 91c65244cc..be93450dc1 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #include #include @@ -90,7 +91,8 @@ class ViewRegistry default: // Unreachable: all AggregationType enum values are handled above - std::abort(); + assert(false && "Unreachable: unhandled AggregationType"); + valid = false; } if (!valid)