Skip to content

[Metrics SDK] Fix histogram views rejected when only aggregation_cardinality_limit is set #4314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
om7057 wants to merge 13 commits into
base: main
Choose a base branch
from
Open

[Metrics SDK] Fix histogram views rejected when only aggregation_cardinality_limit is set #4314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6f17c56
Fix histogram views rejected when only aggregation_cardinality_limit …
om7057 Jul 27, 2026
6b87ec4
Add CHANGELOG entry for PR #4314
om7057 Jul 27, 2026
2ae3755
fix(metrics): add default_aggregation include to sdk_builder_test
om7057 Jul 27, 2026
c313e7f
test(metrics): add Base2ExponentialHistogram cardinality limit coverage
om7057 Jul 27, 2026
5a3eba8
fix(metrics): remove unreachable Base2ExponentialHistogram branch and…
om7057 Jul 27, 2026
39edf35
Merge branch 'main' into fix/histogram-cardinality-limit
marcalff Jul 27, 2026
3484667
fix(metrics): preserve default histogram boundaries in cardinality-li…
om7057 Jul 28, 2026
25115b2
fix(test): satisfy include-what-you-use for sdk_builder_test.cc
om7057 Jul 28, 2026
9b644c1
Merge branch 'main' into fix/histogram-cardinality-limit
marcalff Jul 28, 2026
725fd5e
Merge branch 'main' into fix/histogram-cardinality-limit
om7057 Jul 29, 2026
509f4b0
Merge branch 'main' into fix/histogram-cardinality-limit
dbarker Jul 30, 2026
d98acee
Trigger CI re-run
om7057 Jul 31, 2026
d207474
Merge branch 'main' into fix/histogram-cardinality-limit
marcalff Jul 31, 2026
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,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
Expand Down
9 changes: 9 additions & 0 deletions sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<double> &DefaultBoundaries()
{
static const std::vector<double> 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<double> boundaries_;
bool record_min_max_ = true;
};
Expand Down
40 changes: 38 additions & 2 deletions sdk/src/configuration/sdk_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1748,8 +1749,43 @@ void SdkBuilder::AddView(
}
else
{
sdk_aggregation_config = std::make_shared<opentelemetry::sdk::metrics::AggregationConfig>(
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<opentelemetry::sdk::metrics::HistogramAggregationConfig>(

@ThomsonTan ThomsonTan Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This fixes the view registration, but it appears to silently collapse the histogram to a single bucket. HistogramAggregationConfig default-constructs boundaries_ as an empty vector, while the histogram constructor chooses between configured and default boundaries based only on whether the config pointer is non-null:

if (ac) { point_data_.boundaries_ = ac->boundaries_; }  // now taken with an empty vector
else    { point_data_.boundaries_ = {0.0, 5.0, ... 10000.0}; }
point_data_.counts_ = std::vector<uint64_t>(point_data_.boundaries_.size() + 1, 0);

In other words, "no config" previously selected the SDK's 15 default boundaries, whereas the synthesized config now means "explicitly use zero boundaries," producing one bucket. Could we preserve the default boundaries when creating this config and add a test that asserts the resulting histogram still has 15 boundaries and 16 buckets?

@lalitb lalitb Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch!

@om7057 om7057 Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ThomsonTan Thank you for pointing it out. HistogramAggregationConfig default-constructs with an empty boundaries_, and the histogram aggregation constructors only fall back to the SDK defaults when the config pointer itself is nullptr, not when it's non-null-but-empty. Since this fallback path synthesizes the config rather than getting it from an explicit aggregation block, it was accidentally saying "use zero boundaries" instead of "use the defaults."

Fixed in 3484667, added HistogramAggregationConfig::DefaultBoundaries() as a single source of truth for the SDK's 15-boundary default list, and pointed the synthesized config at it. While I was in there, I also deduped the two other spots (LongHistogramAggregation/DoubleHistogramAggregation) that had this same literal hardcoded, so there will be no chance of the three copies drifting apart in the future.

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<opentelemetry::sdk::metrics::AggregationConfig>(
stream->aggregation_cardinality_limit);
break;
}
}
}

Expand Down
6 changes: 2 additions & 4 deletions sdk/src/metrics/aggregation/histogram_aggregation.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
{
Expand Down
121 changes: 121 additions & 0 deletions sdk/test/configuration/sdk_builder_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include <gtest/gtest.h>

#include <cstddef>
#include <memory>
#include <string>
#include <utility>
Expand All @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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<config_sdk::ViewConfiguration> MakeCardinalityOnlyViewConfig(
config_sdk::InstrumentType instrument_type,
std::size_t cardinality_limit)
{
auto model = std::make_unique<config_sdk::ViewConfiguration>();
model->selector = std::make_unique<config_sdk::ViewSelectorConfiguration>();
model->selector->instrument_type = instrument_type;

model->stream = std::make_unique<config_sdk::ViewStreamConfiguration>();
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::Registry>();
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);

@ThomsonTan ThomsonTan Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new regression test proves only that the view registers with a HistogramAggregationConfig carrying the limit 42; it never constructs the aggregation or inspects boundaries_. Running the test's exact assertions against the object AddView() builds shows all three pass while the histogram is already degenerate:

EXPECT_NE(aggregation_config, nullptr)   -> PASS
EXPECT_EQ(GetType(), kHistogram)         -> PASS
EXPECT_EQ(cardinality_limit_, 42u)       -> PASS
[not asserted] boundaries_.size()        =  0
[not asserted] bucket count (size + 1)   =  1

So the suite stays green (and codecov reports the changed lines fully covered) with zero boundaries and a single bucket. Please make this a behavioral assertion: call DefaultAggregation::CreateAggregation(...) with the returned config and verify the resulting HistogramPointData keeps the SDK defaults (15 boundaries, 16 bucket counts) alongside the cardinality-limit check, so the test pins what users actually receive rather than the shape of the code.

@om7057 om7057 Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, understood. The test was checking the shape of the code, not what a user actually gets. Updated AddViewHistogramCardinalityLimitOnly to call DefaultAggregation::CreateAggregation() with the config the view ends up holding, then assert on the resulting HistogramPointData: 15 boundaries, 16 buckets. I confirmed this assertion actually catches the regression; reverting the boundaries fix locally makes it fail with boundaries_.size() == 0 / counts_.size() == 1, which is exactly the degenerate case you described.

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<metrics_sdk::HistogramPointData>(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::Registry>();
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);
}
Loading