diff --git a/CHANGELOG.md b/CHANGELOG.md index 71e358a95a..317c4f440f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,8 +15,12 @@ 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) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index e64ce09c9d..088a115f9e 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" @@ -13,6 +14,7 @@ namespace sdk { namespace metrics { + class AggregationConfig { public: @@ -20,6 +22,8 @@ class AggregationConfig : cardinality_limit_(cardinality_limit) {} + virtual AggregationType GetType() const noexcept { return AggregationType::kDefault; } + static const AggregationConfig *GetOrDefault(const AggregationConfig *config) { if (config) @@ -41,6 +45,8 @@ class HistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} + AggregationType GetType() const noexcept override { return AggregationType::kHistogram; } + std::vector boundaries_; bool record_min_max_ = true; }; @@ -53,6 +59,11 @@ class Base2ExponentialHistogramAggregationConfig : public AggregationConfig : AggregationConfig(cardinality_limit) {} + AggregationType GetType() const noexcept override + { + return AggregationType::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_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index fd1e4bee0c..be93450dc1 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -4,13 +4,16 @@ #pragma once #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" #include "opentelemetry/sdk/metrics/view/instrument_selector.h" #include "opentelemetry/sdk/metrics/view/meter_selector.h" @@ -47,6 +50,60 @@ class ViewRegistry { // TBD - thread-safe ? + // Validate parameters + if (!instrument_selector || !meter_selector || !view) + { + 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(); + 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; + + default: + // Unreachable: all AggregationType enum values are handled above + assert(false && "Unreachable: unhandled AggregationType"); + valid = false; + } + + if (!valid) + { + OTEL_INTERNAL_LOG_ERROR( + "[ViewRegistry::AddView] AggregationType and AggregationConfig type mismatch. " + "Ignoring AddView call."); + return; + } + } + 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/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")}; diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 600b843f7d..009c67278f 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -93,3 +93,46 @@ TEST(ViewRegistry, FindNonExistingView) EXPECT_EQ(status, true); #endif } + +// Tests for ViewRegistry::AddView null parameter validation +// These should log errors and ignore the call instead of throwing or aborting + +TEST(ViewRegistry, AddViewWithNullInstrumentSelector) +{ + ViewRegistry registry; + std::unique_ptr meter_selector{new MeterSelector("name", "version", "schema")}; + std::unique_ptr view{new View("test_view")}; + + // Should not throw or abort, just log and ignore + registry.AddView(nullptr, std::move(meter_selector), std::move(view)); +} + +TEST(ViewRegistry, AddViewWithNullMeterSelector) +{ + ViewRegistry registry; + std::unique_ptr instrument_selector{ + new InstrumentSelector(InstrumentType::kCounter, "instrument_name", "unit")}; + std::unique_ptr view{new View("test_view")}; + + // Should not throw or abort, just log and ignore + registry.AddView(std::move(instrument_selector), nullptr, std::move(view)); +} + +TEST(ViewRegistry, AddViewWithNullView) +{ + ViewRegistry registry; + std::unique_ptr instrument_selector{ + new InstrumentSelector(InstrumentType::kCounter, "instrument_name", "unit")}; + std::unique_ptr meter_selector{new MeterSelector("name", "version", "schema")}; + + // 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; + + // Should not throw or abort, just log and ignore + registry.AddView(nullptr, nullptr, nullptr); +}