diff --git a/CHANGELOG.md b/CHANGELOG.md index 4183e772c4..73b7dc94d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,10 @@ Increment the: namespaces [#4302](https://github.com/open-telemetry/opentelemetry-cpp/pull/4302) +* [METRICS SDK] Fix histogram views being rejected when only + `aggregation_cardinality_limit` is set + [#4314](https://github.com/open-telemetry/opentelemetry-cpp/pull/4314) + Breaking changes: * [METRICS SDK] Rename Base2 Exponential Histogram Aggregation config field diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index e96736fca9..8c80715846 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -52,6 +52,15 @@ class HistogramAggregationConfig : public AggregationConfig AggregationType GetType() const noexcept override { return AggregationType::kHistogram; } + // The SDK-specified default bucket boundaries, used when no boundaries are configured. + static const std::vector &DefaultBoundaries() + { + static const std::vector boundaries = {0.0, 5.0, 10.0, 25.0, 50.0, + 75.0, 100.0, 250.0, 500.0, 750.0, + 1000.0, 2500.0, 5000.0, 7500.0, 10000.0}; + return boundaries; + } + std::vector boundaries_; bool record_min_max_ = true; }; diff --git a/sdk/src/configuration/sdk_builder.cc b/sdk/src/configuration/sdk_builder.cc index be4e8d6566..2131520653 100644 --- a/sdk/src/configuration/sdk_builder.cc +++ b/sdk/src/configuration/sdk_builder.cc @@ -146,6 +146,7 @@ #include "opentelemetry/sdk/logs/processor.h" #include "opentelemetry/sdk/logs/simple_log_record_processor_factory.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" +#include "opentelemetry/sdk/metrics/aggregation/default_aggregation.h" #include "opentelemetry/sdk/metrics/cardinality_limits.h" #include "opentelemetry/sdk/metrics/export/metric_producer.h" #include "opentelemetry/sdk/metrics/instruments.h" @@ -1748,8 +1749,43 @@ void SdkBuilder::AddView( } else { - sdk_aggregation_config = std::make_shared( - stream->aggregation_cardinality_limit); + // No explicit `aggregation` block was configured, so the view falls back to the + // instrument's default aggregation. ViewRegistry::AddView() rejects a view whose + // AggregationConfig type does not match the (possibly instrument-derived) aggregation + // type, so the config created here must match that same default rather than always + // being a plain AggregationConfig (which only satisfies kSum/kLastValue/kDrop). + auto effective_aggregation_type = sdk_aggregation_type; + if (effective_aggregation_type == opentelemetry::sdk::metrics::AggregationType::kDefault) + { + bool is_monotonic{false}; + effective_aggregation_type = + opentelemetry::sdk::metrics::DefaultAggregation::GetDefaultAggregationType( + sdk_instrument_type, is_monotonic); + } + + switch (effective_aggregation_type) + { + case opentelemetry::sdk::metrics::AggregationType::kHistogram: { + auto histogram_config = + std::make_shared( + stream->aggregation_cardinality_limit); + // A default-constructed HistogramAggregationConfig has empty boundaries_, which + // LongHistogramAggregation/DoubleHistogramAggregation interpret as "use these zero + // boundaries" rather than "no boundaries configured" (that distinction only exists + // when the config pointer itself is null). Since this config is synthesized here + // rather than coming from an explicit `aggregation` block, it must carry the SDK's + // default boundaries to preserve the instrument's default histogram shape. + histogram_config->boundaries_ = + opentelemetry::sdk::metrics::HistogramAggregationConfig::DefaultBoundaries(); + sdk_aggregation_config = histogram_config; + break; + } + + default: + sdk_aggregation_config = std::make_shared( + stream->aggregation_cardinality_limit); + break; + } } } diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index d6b4c90a0b..717ae495e9 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -34,8 +34,7 @@ LongHistogramAggregation::LongHistogramAggregation(const AggregationConfig *aggr } else { - point_data_.boundaries_ = {0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, - 500.0, 750.0, 1000.0, 2500.0, 5000.0, 7500.0, 10000.0}; + point_data_.boundaries_ = HistogramAggregationConfig::DefaultBoundaries(); } if (ac) @@ -115,8 +114,7 @@ DoubleHistogramAggregation::DoubleHistogramAggregation(const AggregationConfig * } else { - point_data_.boundaries_ = {0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, - 500.0, 750.0, 1000.0, 2500.0, 5000.0, 7500.0, 10000.0}; + point_data_.boundaries_ = HistogramAggregationConfig::DefaultBoundaries(); } if (ac) { diff --git a/sdk/test/configuration/sdk_builder_test.cc b/sdk/test/configuration/sdk_builder_test.cc index 461f722df9..178d91ad67 100644 --- a/sdk/test/configuration/sdk_builder_test.cc +++ b/sdk/test/configuration/sdk_builder_test.cc @@ -3,6 +3,7 @@ #include +#include #include #include #include @@ -16,6 +17,7 @@ #include "opentelemetry/sdk/configuration/always_on_sampler_configuration.h" #include "opentelemetry/sdk/configuration/extension_push_metric_exporter_builder.h" #include "opentelemetry/sdk/configuration/extension_push_metric_exporter_configuration.h" +#include "opentelemetry/sdk/configuration/instrument_type.h" #include "opentelemetry/sdk/configuration/logger_config_configuration.h" #include "opentelemetry/sdk/configuration/logger_configurator_configuration.h" #include "opentelemetry/sdk/configuration/logger_matcher_and_config_configuration.h" @@ -30,11 +32,23 @@ #include "opentelemetry/sdk/configuration/span_limits_configuration.h" #include "opentelemetry/sdk/configuration/trace_id_ratio_based_sampler_configuration.h" #include "opentelemetry/sdk/configuration/tracer_provider_configuration.h" +#include "opentelemetry/sdk/configuration/view_configuration.h" +#include "opentelemetry/sdk/configuration/view_selector_configuration.h" +#include "opentelemetry/sdk/configuration/view_stream_configuration.h" +#include "opentelemetry/nostd/function_ref.h" +#include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" #include "opentelemetry/sdk/instrumentationscope/scope_configurator.h" #include "opentelemetry/sdk/logs/logger_config.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" +#include "opentelemetry/sdk/metrics/aggregation/default_aggregation.h" +#include "opentelemetry/sdk/metrics/data/point_data.h" +#include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/metric_reader.h" +#include "opentelemetry/sdk/metrics/view/view.h" +#include "opentelemetry/sdk/metrics/view/view_registry.h" #include "opentelemetry/sdk/resource/resource.h" #include "opentelemetry/sdk/trace/sampler.h" #include "opentelemetry/sdk/trace/span_limits.h" @@ -260,3 +274,110 @@ TEST(SdkBuilder, CreatePeriodicMetricReader) EXPECT_EQ(captured->timeout, model.timeout); EXPECT_TRUE(captured->exporter != nullptr); } + +namespace +{ + +// Builds a ViewConfiguration selecting the given instrument type, with only +// aggregation_cardinality_limit set on the stream (no explicit `aggregation` block). +std::unique_ptr MakeCardinalityOnlyViewConfig( + config_sdk::InstrumentType instrument_type, + std::size_t cardinality_limit) +{ + auto model = std::make_unique(); + model->selector = std::make_unique(); + model->selector->instrument_type = instrument_type; + + model->stream = std::make_unique(); + model->stream->aggregation_cardinality_limit = cardinality_limit; + + return model; +} + +} // namespace + +TEST(SdkBuilder, AddViewHistogramCardinalityLimitOnly) +{ + namespace metrics_sdk = opentelemetry::sdk::metrics; + + auto model = MakeCardinalityOnlyViewConfig(config_sdk::InstrumentType::histogram, 42); + + auto registry = std::make_shared(); + config_sdk::SdkBuilder builder(registry); + + metrics_sdk::ViewRegistry view_registry; + builder.AddView(&view_registry, model); + + metrics_sdk::InstrumentDescriptor instrument_descriptor{ + "", "", "", metrics_sdk::InstrumentType::kHistogram, metrics_sdk::InstrumentValueType::kLong}; + auto instrumentation_scope = scope_sdk::InstrumentationScope::Create(""); + + int matched = 0; + view_registry.FindViews( + instrument_descriptor, *instrumentation_scope, [&](const metrics_sdk::View &view) { + matched++; + // The view must not be rejected: it should carry a + // HistogramAggregationConfig (not a plain AggregationConfig), since + // the instrument's default aggregation for kHistogram is kHistogram. + auto *aggregation_config = view.GetAggregationConfig(); + EXPECT_NE(aggregation_config, nullptr); + if (aggregation_config) + { + EXPECT_EQ(aggregation_config->GetType(), metrics_sdk::AggregationType::kHistogram); + EXPECT_EQ(aggregation_config->cardinality_limit_, 42u); + + // Pin what users actually receive: building the aggregation from this config + // must keep the SDK's default bucket boundaries, not silently collapse to a + // single bucket. A default-constructed HistogramAggregationConfig has empty + // boundaries_, which the aggregation takes literally (as opposed to a null + // config pointer, which falls back to the default boundaries), so AddView() + // must populate boundaries_ explicitly. + auto aggregation = metrics_sdk::DefaultAggregation::CreateAggregation( + metrics_sdk::AggregationType::kHistogram, instrument_descriptor, aggregation_config); + EXPECT_NE(aggregation, nullptr); + if (aggregation) + { + auto histogram_data = + opentelemetry::nostd::get(aggregation->ToPoint()); + EXPECT_EQ(histogram_data.boundaries_.size(), 15u); + EXPECT_EQ(histogram_data.counts_.size(), 16u); + } + } + return true; + }); + + EXPECT_EQ(matched, 1); +} + +TEST(SdkBuilder, AddViewCounterCardinalityLimitOnly) +{ + namespace metrics_sdk = opentelemetry::sdk::metrics; + + auto model = MakeCardinalityOnlyViewConfig(config_sdk::InstrumentType::counter, 7); + + auto registry = std::make_shared(); + config_sdk::SdkBuilder builder(registry); + + metrics_sdk::ViewRegistry view_registry; + builder.AddView(&view_registry, model); + + metrics_sdk::InstrumentDescriptor instrument_descriptor{ + "", "", "", metrics_sdk::InstrumentType::kCounter, metrics_sdk::InstrumentValueType::kLong}; + auto instrumentation_scope = scope_sdk::InstrumentationScope::Create(""); + + int matched = 0; + view_registry.FindViews( + instrument_descriptor, *instrumentation_scope, [&](const metrics_sdk::View &view) { + matched++; + auto *aggregation_config = view.GetAggregationConfig(); + EXPECT_NE(aggregation_config, nullptr); + if (aggregation_config) + { + EXPECT_EQ(aggregation_config->GetType(), metrics_sdk::AggregationType::kDefault); + EXPECT_EQ(aggregation_config->cardinality_limit_, 7u); + } + return true; + }); + + EXPECT_EQ(matched, 1); +}