[METRICS] Add tag to AggregationConfig for aggregation type validation

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/meter_context.h

@@ -136,7 +136,7 @@ class MeterContext : public std::enable_shared_from_this<MeterContext>
    */
   void AddView(std::unique_ptr<InstrumentSelector> instrument_selector,
                std::unique_ptr<MeterSelector> meter_selector,
-               std::unique_ptr<View> view) noexcept;
+               std::unique_ptr<View> view);
 
 #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
 

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

@@ -4,6 +4,7 @@
 #pragma once
 
 #include <memory>
+#include <stdexcept>
 #include <string>
 
 #include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h"

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

@@ -5,12 +5,14 @@
 
 #include <algorithm>
 #include <memory>
+#include <stdexcept>
 #include <string>
 #include <utility>
 #include <vector>
 
 #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,61 @@ 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;
+
+        default:
+          // Unreachable: all AggregationType enum values are handled above
+          std::abort();
+      }
+
+      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<RegisteredView>(new RegisteredView{
         std::move(instrument_selector), std::move(meter_selector), std::move(view)});
     registered_views_.push_back(std::move(registered_view));

sdk/src/metrics/meter_context.cc

@@ -106,7 +106,7 @@ void MeterContext::AddMetricReader(std::shared_ptr<MetricReader> reader,
 
 void MeterContext::AddView(std::unique_ptr<InstrumentSelector> instrument_selector,
                            std::unique_ptr<MeterSelector> meter_selector,
-                           std::unique_ptr<View> view) noexcept
+                           std::unique_ptr<View> view)
 {
   views_->AddView(std::move(instrument_selector), std::move(meter_selector), std::move(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

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #include <gtest/gtest.h>
+#include <stdexcept>
 #include <string>
 #include <utility>
 
@@ -93,3 +94,65 @@ TEST(ViewRegistry, FindNonExistingView)
   EXPECT_EQ(status, true);
 #endif
 }
+
+// Tests for ViewRegistry::AddView null parameter validation
+
+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")};
+
+#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
+}
+
+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")};
+
+#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
+}
+
+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")};
+
+#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
+}
+
+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
+}