Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a2c48c6
Add tag to AggregationConfig for type checking
ThomsonTan Oct 29, 2025
d2330d1
Add test
ThomsonTan Oct 31, 2025
177d85a
Fix format
ThomsonTan Oct 31, 2025
db221d8
Address feedback
ThomsonTan Oct 31, 2025
7ec33ec
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Oct 31, 2025
4d9331c
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Nov 3, 2025
5dfc194
Support build without exception
ThomsonTan Nov 3, 2025
2c4266b
Remove logging
ThomsonTan Nov 3, 2025
6f04f06
Handle unittest in noexception build
ThomsonTan Nov 3, 2025
363eb46
Fix EXPECT_NO_THROW for no-exception build
ThomsonTan Nov 4, 2025
b500f61
Fix iwyu
ThomsonTan Nov 4, 2025
af73f3a
Move check to AddView
ThomsonTan Nov 5, 2025
f28235c
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Nov 5, 2025
6179d6f
Handle kDefault in AddView
ThomsonTan Nov 5, 2025
d3b4c42
Fix meter provider test
ThomsonTan Nov 5, 2025
8e2a1b0
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Nov 5, 2025
9dd5eae
Fix iwyu
ThomsonTan Nov 5, 2025
36ef49a
Remove noexcept on AddView
ThomsonTan Nov 5, 2025
5e96531
Add changelog
ThomsonTan Nov 6, 2025
f30cea6
Update changelog
ThomsonTan Nov 6, 2025
f150c4a
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Nov 6, 2025
4b27470
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Nov 6, 2025
4400651
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Nov 7, 2025
a530053
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Nov 10, 2025
01889b2
Log View validation error and ignore it
ThomsonTan Nov 10, 2025
3867d1d
Add back noexcept on AddView
ThomsonTan Nov 10, 2025
c1c5d0a
Add noexcept back to AddView implementation
ThomsonTan Nov 10, 2025
83f6394
Merge branch 'main' into add_tag_to_aggregationconfig
ThomsonTan Nov 14, 2025
b61e528
Address feedback
ThomsonTan Nov 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <vector>

#include "opentelemetry/sdk/metrics/instruments.h"
#include "opentelemetry/sdk/metrics/state/attributes_hashmap.h"
#include "opentelemetry/version.h"

Expand All @@ -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 { return AggregationType::kDefault; }

static const AggregationConfig *GetOrDefault(const AggregationConfig *config)
{
if (config)
Expand All @@ -41,6 +45,8 @@ class HistogramAggregationConfig : public AggregationConfig
: AggregationConfig(cardinality_limit)
{}

AggregationType GetType() const override { return AggregationType::kHistogram; }

std::vector<double> boundaries_;
bool record_min_max_ = true;
};
Expand All @@ -53,6 +59,8 @@ class Base2ExponentialHistogramAggregationConfig : public AggregationConfig
: AggregationConfig(cardinality_limit)
{}

AggregationType GetType() const override { return AggregationType::kBase2ExponentialHistogram; }

size_t max_buckets_ = 160;
int32_t max_scale_ = 20;
bool record_min_max_ = true;
Expand Down
31 changes: 30 additions & 1 deletion sdk/include/opentelemetry/sdk/metrics/view/view.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#pragma once

#include <memory>
#include <stdexcept>
#include <string>

#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h"
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that aggregation_config contains its proper type, can the aggregation_type parameter be removed instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not for now because the common AggregationConfig can still handle multiple aggregation types, like kDefault, kSum, etc. It is not 1:1 mapping.

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)
{
throw std::invalid_argument("AggregationType and AggregationConfig type mismatch");
}
}
}

virtual ~View() = default;

Expand Down
56 changes: 56 additions & 0 deletions sdk/test/metrics/view_registry_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -93,3 +94,58 @@ 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<HistogramAggregationConfig>();
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<HistogramAggregationConfig>();
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<HistogramAggregationConfig>();

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<AggregationConfig>();

EXPECT_THROW(
{ View view("test_histogram", "description", AggregationType::kHistogram, default_config); },
std::invalid_argument);
}
Loading