diff --git a/CHANGELOG.md b/CHANGELOG.md index e7934aae3f..906d4c3e2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,16 @@ Increment the: `GetSpan(context)->GetContext()` incurs, and use it at existing call sites. [#4254](https://github.com/open-telemetry/opentelemetry-cpp/pull/4254) +* [METRICS SDK] Validate Base2 Exponential Histogram Aggregation config + [#4253](https://github.com/open-telemetry/opentelemetry-cpp/pull/4253) + +Breaking changes: + +* [METRICS SDK] Rename Base2 Exponential Histogram Aggregation config field + [#4253](https://github.com/open-telemetry/opentelemetry-cpp/pull/4253) + * The public configuration member `max_buckets_` was renamed to `max_size_` to + match the configuration schema. Please adjust SDK configuration accordingly. + ## [1.28.0] 2026-07-16 * [RELEASE] Bump main branch to 1.28.0-dev diff --git a/examples/metrics_simple/metrics_ostream.cc b/examples/metrics_simple/metrics_ostream.cc index 46a775d32f..1834b40b9f 100644 --- a/examples/metrics_simple/metrics_ostream.cc +++ b/examples/metrics_simple/metrics_ostream.cc @@ -124,7 +124,7 @@ void InitMetrics(const std::string &name) new metrics_sdk::Base2ExponentialHistogramAggregationConfig); histogram_base2_aggregation_config->max_scale_ = 3; histogram_base2_aggregation_config->record_min_max_ = true; - histogram_base2_aggregation_config->max_buckets_ = 100; + histogram_base2_aggregation_config->max_size_ = 100; std::shared_ptr base2_aggregation_config( std::move(histogram_base2_aggregation_config)); diff --git a/sdk/include/opentelemetry/sdk/configuration/base2_exponential_bucket_histogram_aggregation_configuration.h b/sdk/include/opentelemetry/sdk/configuration/base2_exponential_bucket_histogram_aggregation_configuration.h index e684eadae7..adfaac3f94 100644 --- a/sdk/include/opentelemetry/sdk/configuration/base2_exponential_bucket_histogram_aggregation_configuration.h +++ b/sdk/include/opentelemetry/sdk/configuration/base2_exponential_bucket_histogram_aggregation_configuration.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include "opentelemetry/sdk/configuration/aggregation_configuration.h" #include "opentelemetry/sdk/configuration/aggregation_configuration_visitor.h" @@ -20,19 +21,16 @@ namespace configuration class Base2ExponentialBucketHistogramAggregationConfiguration : public AggregationConfiguration { public: - // TODO: max_scale range is [-10, 20]. Negative values require GetSignedInteger in the parser and - // changing the type of max_scale to an int32_t to match the - // Base2ExponentialHistogramAggregationConfig. - static constexpr std::size_t kDefaultMaxScale = 20; // schema: minimum -10, maximum 20 - static constexpr std::size_t kDefaultMaxSize = 160; - static constexpr bool kDefaultRecordMinMax = true; + static constexpr std::int32_t kDefaultMaxScale = 20; // schema: minimum -10, maximum 20 + static constexpr std::size_t kDefaultMaxSize = 160; + static constexpr bool kDefaultRecordMinMax = true; void Accept(AggregationConfigurationVisitor *visitor) const override { visitor->VisitBase2ExponentialBucketHistogram(this); } - std::size_t max_scale{kDefaultMaxScale}; + std::int32_t max_scale{kDefaultMaxScale}; std::size_t max_size{kDefaultMaxSize}; bool record_min_max{kDefaultRecordMinMax}; }; diff --git a/sdk/include/opentelemetry/sdk/configuration/document_node.h b/sdk/include/opentelemetry/sdk/configuration/document_node.h index 7a725a3116..2dc715eec9 100644 --- a/sdk/include/opentelemetry/sdk/configuration/document_node.h +++ b/sdk/include/opentelemetry/sdk/configuration/document_node.h @@ -3,6 +3,7 @@ #pragma once +#include #include #include @@ -49,6 +50,9 @@ class DocumentNode virtual std::size_t GetRequiredInteger(const std::string &name) const = 0; virtual std::size_t GetInteger(const std::string &name, std::size_t default_value) const = 0; + virtual std::int64_t GetSignedInteger(const std::string &name, + std::int64_t default_value) const = 0; + virtual double GetRequiredDouble(const std::string &name) const = 0; virtual double GetDouble(const std::string &name, double default_value) const = 0; @@ -71,6 +75,7 @@ class DocumentNode bool BooleanFromString(const std::string &value) const; std::size_t IntegerFromString(const std::string &value) const; + std::int64_t SignedIntegerFromString(const std::string &value) const; double DoubleFromString(const std::string &value) const; }; diff --git a/sdk/include/opentelemetry/sdk/configuration/ryml_document_node.h b/sdk/include/opentelemetry/sdk/configuration/ryml_document_node.h index 23e559977a..1d37b0a7d2 100644 --- a/sdk/include/opentelemetry/sdk/configuration/ryml_document_node.h +++ b/sdk/include/opentelemetry/sdk/configuration/ryml_document_node.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #include #include @@ -49,6 +50,8 @@ class RymlDocumentNode : public DocumentNode std::size_t GetRequiredInteger(const std::string &name) const override; std::size_t GetInteger(const std::string &name, std::size_t default_value) const override; + std::int64_t GetSignedInteger(const std::string &name, std::int64_t default_value) const override; + double GetRequiredDouble(const std::string &name) const override; double GetDouble(const std::string &name, double default_value) const override; diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index 33792ca688..e96736fca9 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -56,6 +56,13 @@ class HistogramAggregationConfig : public AggregationConfig bool record_min_max_ = true; }; +// Valid ranges per the declarative configuration schema; the schema defines no maximum for +// max_size. +// https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema/meter_provider.yaml +constexpr std::int32_t kMaxScaleMin = -10; +constexpr std::int32_t kMaxScaleMax = 20; +constexpr std::size_t kMaxSizeMin = 2; + class Base2ExponentialHistogramAggregationConfig : public AggregationConfig { public: @@ -69,7 +76,7 @@ class Base2ExponentialHistogramAggregationConfig : public AggregationConfig return AggregationType::kBase2ExponentialHistogram; } - size_t max_buckets_ = 160; + size_t max_size_ = 160; int32_t max_scale_ = 20; bool record_min_max_ = true; }; diff --git a/sdk/include/opentelemetry/sdk/metrics/exemplar/reservoir_utils.h b/sdk/include/opentelemetry/sdk/metrics/exemplar/reservoir_utils.h index 6ca88411af..f820eddc25 100644 --- a/sdk/include/opentelemetry/sdk/metrics/exemplar/reservoir_utils.h +++ b/sdk/include/opentelemetry/sdk/metrics/exemplar/reservoir_utils.h @@ -39,8 +39,7 @@ static inline size_t GetSimpleFixedReservoirDefaultSize(const AggregationType ag { const auto *histogram_agg_config = static_cast(agg_config); - return (std::min)(kMaxBase2ExponentialHistogramReservoirSize, - histogram_agg_config->max_buckets_); + return (std::min)(kMaxBase2ExponentialHistogramReservoirSize, histogram_agg_config->max_size_); } return SimpleFixedSizeExemplarReservoir::kDefaultSimpleReservoirSize; diff --git a/sdk/src/configuration/configuration_parser.cc b/sdk/src/configuration/configuration_parser.cc index cc83201a97..bfe4dfa373 100644 --- a/sdk/src/configuration/configuration_parser.cc +++ b/sdk/src/configuration/configuration_parser.cc @@ -128,6 +128,7 @@ #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/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/version.h" OPENTELEMETRY_BEGIN_NAMESPACE @@ -1370,8 +1371,24 @@ ConfigurationParser::ParseBase2ExponentialBucketHistogramAggregationConfiguratio using Config = Base2ExponentialBucketHistogramAggregationConfiguration; auto model = std::make_unique(); - model->max_scale = node->GetInteger("max_scale", Config::kDefaultMaxScale); - model->max_size = node->GetInteger("max_size", Config::kDefaultMaxSize); + std::int64_t max_scale = node->GetSignedInteger("max_scale", Config::kDefaultMaxScale); + if (max_scale < opentelemetry::sdk::metrics::kMaxScaleMin || + max_scale > opentelemetry::sdk::metrics::kMaxScaleMax) + { + std::string message("Illegal max_scale: "); + message.append(std::to_string(max_scale)); + throw InvalidSchemaException(node->Location(), message); + } + model->max_scale = static_cast(max_scale); + + model->max_size = node->GetInteger("max_size", Config::kDefaultMaxSize); + if (model->max_size < opentelemetry::sdk::metrics::kMaxSizeMin) + { + std::string message("Illegal max_size: "); + message.append(std::to_string(model->max_size)); + throw InvalidSchemaException(node->Location(), message); + } + model->record_min_max = node->GetBoolean("record_min_max", Config::kDefaultRecordMinMax); return model; diff --git a/sdk/src/configuration/document_node.cc b/sdk/src/configuration/document_node.cc index f5aafe9f76..61af16de90 100644 --- a/sdk/src/configuration/document_node.cc +++ b/sdk/src/configuration/document_node.cc @@ -2,7 +2,9 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include +#include #include #include "opentelemetry/sdk/common/env_variables.h" @@ -250,6 +252,34 @@ size_t DocumentNode::IntegerFromString(const std::string &value) const return val; } +std::int64_t DocumentNode::SignedIntegerFromString(const std::string &value) const +{ + try + { + std::size_t pos = 0; + std::int64_t val = std::stoll(value, &pos); + if (pos != value.length()) + { + std::string message("Illegal integer value: "); + message.append(value); + throw InvalidSchemaException(Location(), message); + } + return val; + } + catch (const std::invalid_argument &) + { + std::string message("Illegal integer value: "); + message.append(value); + throw InvalidSchemaException(Location(), message); + } + catch (const std::out_of_range &) + { + std::string message("Illegal integer value: "); + message.append(value); + throw InvalidSchemaException(Location(), message); + } +} + double DocumentNode::DoubleFromString(const std::string &value) const { const char *ptr = value.c_str(); diff --git a/sdk/src/configuration/ryml_document_node.cc b/sdk/src/configuration/ryml_document_node.cc index ea5986ee3c..062b3214d1 100644 --- a/sdk/src/configuration/ryml_document_node.cc +++ b/sdk/src/configuration/ryml_document_node.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include #include #include @@ -281,6 +282,32 @@ size_t RymlDocumentNode::GetInteger(const std::string &name, size_t default_valu return IntegerFromString(value); } +std::int64_t RymlDocumentNode::GetSignedInteger(const std::string &name, + std::int64_t default_value) const +{ + OTEL_INTERNAL_LOG_DEBUG("RymlDocumentNode::GetSignedInteger(" << name << ", " << default_value + << ")"); + + auto ryml_child = GetRymlChildNode(name); + + if (ryml_child.invalid()) + { + return default_value; + } + + ryml::csubstr view = ryml_child.val(); + std::string value(view.str, view.len); + + value = DoSubstitution(value); + + if (value.empty()) + { + return default_value; + } + + return SignedIntegerFromString(value); +} + double RymlDocumentNode::GetRequiredDouble(const std::string &name) const { OTEL_INTERNAL_LOG_DEBUG("RymlDocumentNode::GetRequiredDouble(" << name << ")"); diff --git a/sdk/src/configuration/sdk_builder.cc b/sdk/src/configuration/sdk_builder.cc index 7e3a9600fc..3d4494e7a6 100644 --- a/sdk/src/configuration/sdk_builder.cc +++ b/sdk/src/configuration/sdk_builder.cc @@ -1619,8 +1619,8 @@ SdkBuilder::CreateBase2ExponentialBucketHistogramAggregation( auto sdk = std::make_unique(); - sdk->max_buckets_ = model->max_size; - sdk->max_scale_ = static_cast(model->max_scale); + sdk->max_size_ = model->max_size; + sdk->max_scale_ = model->max_scale; sdk->record_min_max_ = model->record_min_max; return sdk; diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index 7909b0a384..4803a4674f 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -124,8 +124,26 @@ Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( ac = &default_config; } - point_data_.max_buckets_ = (std::max)(ac->max_buckets_, static_cast(2)); - point_data_.scale_ = ac->max_scale_; + size_t max_size = ac->max_size_; + if (max_size < kMaxSizeMin) + { + OTEL_INTERNAL_LOG_WARN("[Base2ExponentialHistogramAggregation] max_size " + << max_size << " is less than " << kMaxSizeMin << "; using default " + << default_config.max_size_); + max_size = default_config.max_size_; + } + + int32_t max_scale = ac->max_scale_; + if (max_scale < kMaxScaleMin || max_scale > kMaxScaleMax) + { + OTEL_INTERNAL_LOG_WARN("[Base2ExponentialHistogramAggregation] max_scale " + << max_scale << " is out of range [" << kMaxScaleMin << ", " + << kMaxScaleMax << "]; using default " << default_config.max_scale_); + max_scale = default_config.max_scale_; + } + + point_data_.max_buckets_ = max_size; + point_data_.scale_ = max_scale; point_data_.record_min_max_ = ac->record_min_max_; point_data_.min_ = (std::numeric_limits::max)(); point_data_.max_ = (std::numeric_limits::min)(); diff --git a/sdk/test/configuration/yaml_metrics_test.cc b/sdk/test/configuration/yaml_metrics_test.cc index 7272d2fc02..304a7301a0 100644 --- a/sdk/test/configuration/yaml_metrics_test.cc +++ b/sdk/test/configuration/yaml_metrics_test.cc @@ -925,7 +925,7 @@ file_format: "1.0-metrics" stream: aggregation: base2_exponential_bucket_histogram: - max_scale: 40 + max_scale: 10 max_size: 320 record_min_max: false )"; @@ -943,12 +943,108 @@ file_format: "1.0-metrics" auto *base2_exponential_bucket_histogram = reinterpret_cast< opentelemetry::sdk::configuration::Base2ExponentialBucketHistogramAggregationConfiguration *>( aggregation); - ASSERT_EQ(base2_exponential_bucket_histogram->max_scale, 40); + ASSERT_EQ(base2_exponential_bucket_histogram->max_scale, 10); ASSERT_EQ(base2_exponential_bucket_histogram->max_size, 320); ASSERT_EQ(base2_exponential_bucket_histogram->record_min_max, false); ASSERT_EQ(view->stream->attribute_keys, nullptr); } +TEST(YamlMetrics, stream_aggregation_base2_exponential_bucket_histogram_min_values) +{ + std::string yaml = R"( +file_format: "1.0-metrics" +meter_provider: + readers: + - periodic: + exporter: + console: + views: + - selector: + stream: + aggregation: + base2_exponential_bucket_histogram: + max_scale: -10 + max_size: 2 +)"; + + auto config = DoParse(yaml); + ASSERT_NE(config, nullptr); + ASSERT_NE(config->meter_provider, nullptr); + ASSERT_EQ(config->meter_provider->views.size(), 1); + auto *view = config->meter_provider->views[0].get(); + ASSERT_NE(view, nullptr); + ASSERT_NE(view->stream, nullptr); + ASSERT_NE(view->stream->aggregation, nullptr); + auto *base2_exponential_bucket_histogram = reinterpret_cast< + opentelemetry::sdk::configuration::Base2ExponentialBucketHistogramAggregationConfiguration *>( + view->stream->aggregation.get()); + ASSERT_EQ(base2_exponential_bucket_histogram->max_scale, -10); + ASSERT_EQ(base2_exponential_bucket_histogram->max_size, 2); +} + +TEST(YamlMetrics, stream_aggregation_base2_exponential_bucket_histogram_max_scale_too_small) +{ + std::string yaml = R"( +file_format: "1.0-metrics" +meter_provider: + readers: + - periodic: + exporter: + console: + views: + - selector: + stream: + aggregation: + base2_exponential_bucket_histogram: + max_scale: -11 +)"; + + auto config = DoParse(yaml); + ASSERT_EQ(config, nullptr); +} + +TEST(YamlMetrics, stream_aggregation_base2_exponential_bucket_histogram_max_scale_too_large) +{ + std::string yaml = R"( +file_format: "1.0-metrics" +meter_provider: + readers: + - periodic: + exporter: + console: + views: + - selector: + stream: + aggregation: + base2_exponential_bucket_histogram: + max_scale: 21 +)"; + + auto config = DoParse(yaml); + ASSERT_EQ(config, nullptr); +} + +TEST(YamlMetrics, stream_aggregation_base2_exponential_bucket_histogram_max_size_too_small) +{ + std::string yaml = R"( +file_format: "1.0-metrics" +meter_provider: + readers: + - periodic: + exporter: + console: + views: + - selector: + stream: + aggregation: + base2_exponential_bucket_histogram: + max_size: 1 +)"; + + auto config = DoParse(yaml); + ASSERT_EQ(config, nullptr); +} + TEST(YamlMetrics, stream_aggregation_last_value) { std::string yaml = R"( diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index e8c7ab0dcb..8cd8a1a4ec 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -51,11 +51,11 @@ uint64_t SumAllBuckets(const Base2ExponentialHistogramPointData &point) return total; } -Base2ExponentialHistogramAggregationConfig MakeAggregationConfig(int max_scale, size_t max_buckets) +Base2ExponentialHistogramAggregationConfig MakeAggregationConfig(int max_scale, size_t max_size) { Base2ExponentialHistogramAggregationConfig config; - config.max_scale_ = max_scale; - config.max_buckets_ = max_buckets; + config.max_scale_ = max_scale; + config.max_size_ = max_size; return config; } @@ -315,7 +315,7 @@ TEST(Aggregation, Base2ExponentialHistogramAggregation) auto MAX_BUCKETS0 = 7; Base2ExponentialHistogramAggregationConfig scale0_config; scale0_config.max_scale_ = SCALE0; - scale0_config.max_buckets_ = MAX_BUCKETS0; + scale0_config.max_size_ = MAX_BUCKETS0; scale0_config.record_min_max_ = true; Base2ExponentialHistogramAggregation scale0_aggr(&scale0_config); auto point = scale0_aggr.ToPoint(); @@ -388,7 +388,7 @@ TEST(Aggregation, Base2ExponentialHistogramAggregation) Base2ExponentialHistogramAggregationConfig scale1_config; scale1_config.max_scale_ = 1; - scale1_config.max_buckets_ = 14; + scale1_config.max_size_ = 14; scale1_config.record_min_max_ = true; Base2ExponentialHistogramAggregation scale1_aggr(&scale1_config); @@ -441,11 +441,41 @@ TEST(Aggregation, Base2ExponentialHistogramAggregation) EXPECT_EQ(diffd_point.positive_buckets_->Get(2), 1); } +TEST(Aggregation, Base2ExponentialHistogramAggregationInvalidConfigFallsBackToDefault) +{ + const Base2ExponentialHistogramAggregationConfig default_config; + + auto point_of = [](const Base2ExponentialHistogramAggregationConfig &config) { + Base2ExponentialHistogramAggregation aggr(&config); + return nostd::get(aggr.ToPoint()); + }; + + { + auto point = point_of(MakeAggregationConfig(kMaxScaleMax + 1, 100)); + EXPECT_EQ(point.scale_, default_config.max_scale_); + EXPECT_EQ(point.max_buckets_, 100); + } + { + auto point = point_of(MakeAggregationConfig(kMaxScaleMin - 1, 100)); + EXPECT_EQ(point.scale_, default_config.max_scale_); + } + { + auto point = point_of(MakeAggregationConfig(0, kMaxSizeMin - 1)); + EXPECT_EQ(point.max_buckets_, default_config.max_size_); + EXPECT_EQ(point.scale_, 0); + } + { + auto point = point_of(MakeAggregationConfig(kMaxScaleMin, kMaxSizeMin)); + EXPECT_EQ(point.scale_, kMaxScaleMin); + EXPECT_EQ(point.max_buckets_, kMaxSizeMin); + } +} + TEST(Aggregation, Base2ExponentialHistogramAggregationMerge) { Base2ExponentialHistogramAggregationConfig config; config.max_scale_ = 10; - config.max_buckets_ = 100; + config.max_size_ = 100; config.record_min_max_ = true; Base2ExponentialHistogramAggregation aggr(&config); @@ -468,7 +498,7 @@ TEST(Aggregation, Base2ExponentialHistogramAggregationMerge) ASSERT_DOUBLE_EQ(aggr_point.sum_, expected_sum); ASSERT_EQ(aggr_point.zero_count_, 0); ASSERT_GT(aggr_point.scale_, -10); - ASSERT_EQ(aggr_point.max_buckets_, config.max_buckets_); + ASSERT_EQ(aggr_point.max_buckets_, config.max_size_); auto test_merge = [](const std::unique_ptr &merged_aggr, int expected_count, double expected_sum, int expected_zero_count, int expected_scale, @@ -943,7 +973,7 @@ TEST(Aggregation, Base2ExponentialHistogramAggregationDefaultConfigMerge) ExpectCountInvariant(pb.count_, pb, "b"); EXPECT_LE(pa.scale_, 20); EXPECT_LE(pb.scale_, 20); - EXPECT_EQ(pa.max_buckets_, config.max_buckets_); + EXPECT_EQ(pa.max_buckets_, config.max_size_); EXPECT_GT(pa.max_, pa.min_); EXPECT_GT(pb.max_, pb.min_); diff --git a/sdk/test/metrics/histogram_aggregation_test.cc b/sdk/test/metrics/histogram_aggregation_test.cc index 7c74b8b385..23f14f095d 100644 --- a/sdk/test/metrics/histogram_aggregation_test.cc +++ b/sdk/test/metrics/histogram_aggregation_test.cc @@ -195,8 +195,8 @@ std::shared_ptr MakeBase2ExponentialHistogramViewProvider( meter_provider->AddMetricReader(reader); Base2ExponentialHistogramAggregationConfig config; - config.max_scale_ = 5; - config.max_buckets_ = 160; + config.max_scale_ = 5; + config.max_size_ = 160; auto view = std::make_unique("exponential_histogram", "exponential_histogram_description", AggregationType::kBase2ExponentialHistogram,