[METRICS] Add tag to AggregationConfig for aggregation type validation (#3732)

Author: ThomsonTan

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)
 

sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h

@@ -5,6 +5,7 @@
 
 #include <vector>
 
+#include "opentelemetry/sdk/metrics/instruments.h"
 #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h"
 #include "opentelemetry/version.h"
 
@@ -13,13 +14,16 @@ namespace sdk
 {
 namespace metrics
 {
+
 class AggregationConfig
 {
 public:
   AggregationConfig(size_t cardinality_limit = kAggregationCardinalityLimit)
       : 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<double> 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;

sdk/include/opentelemetry/sdk/metrics/view/view_registry.h

@@ -4,13 +4,16 @@
 #pragma once
 
 #include <algorithm>
+#include <cassert>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
 #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<RegisteredView>(new RegisteredView{
         std::move(instrument_selector), std::move(meter_selector), std::move(view)});
     registered_views_.push_back(std::move(registered_view));

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> view{std::unique_ptr<View>()};
-  // 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<MetricReader> reader{new MockMetricReader(std::move(exporter))};
   mp1.AddMetricReader(std::move(reader));
 
-  std::unique_ptr<View> view{std::unique_ptr<View>()};
+  std::unique_ptr<View> view{new View("test_view")};
   std::unique_ptr<InstrumentSelector> instrument_selector{
       new InstrumentSelector(InstrumentType::kCounter, "instru1", "unit1")};
   std::unique_ptr<MeterSelector> meter_selector{new MeterSelector("name1", "version1", "schema1")};

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<MeterSelector> meter_selector{new MeterSelector("name", "version", "schema")};
+  std::unique_ptr<View> 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<InstrumentSelector> instrument_selector{
+      new InstrumentSelector(InstrumentType::kCounter, "instrument_name", "unit")};
+  std::unique_ptr<View> 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<InstrumentSelector> instrument_selector{
+      new InstrumentSelector(InstrumentType::kCounter, "instrument_name", "unit")};
+  std::unique_ptr<MeterSelector> 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);
+}