[Metrics SDK] Fix histogram views rejected when only aggregation_cardinality_limit is set - #4314
[Metrics SDK] Fix histogram views rejected when only aggregation_cardinality_limit is set#4314om7057 wants to merge 12 commits into
Conversation
…is set When a metrics view's stream configuration set aggregation_cardinality_limit without an explicit aggregation block, SdkBuilder::AddView() always built a plain AggregationConfig. For histogram (and base2 exponential histogram) instruments this mismatched the type ViewRegistry::AddView() expects for the instrument's default aggregation, so the whole view was silently rejected and the configured cardinality limit never took effect. AddView() now resolves the instrument's effective aggregation type (falling back to the instrument-derived default, same as ViewRegistry does) and constructs the matching AggregationConfig subclass before applying the cardinality limit. Follow-up to open-telemetry#4188, addressing review feedback left after that PR merged.
|
cc @lalitb @marcalff follow-up to #4188 as requested here. Fixes the histogram-view rejection when only aggregation_cardinality_limit is set, plus a regression test that reproduces the exact rejection reported. Reader-level cardinality limit enforcement (the other open TODO) is intentionally out of scope here; happy to take that on in a separate PR if you'd like it prioritized before the next release. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4314 +/- ##
==========================================
+ Coverage 81.17% 81.20% +0.04%
==========================================
Files 446 446
Lines 18929 18944 +15
==========================================
+ Hits 15363 15381 +18
+ Misses 3566 3563 -3
🚀 New features to boost your workflow:
|
… fix format The fallback branch in SdkBuilder::AddView() only runs when no explicit aggregation block was set, in which case the aggregation type is always resolved via DefaultAggregation::GetDefaultAggregationType(), which can only return kSum, kHistogram, kLastValue, or kDrop for a given instrument type - never kBase2ExponentialHistogram. The switch case for it was dead code, and the regression test added to cover it referenced instrument type enumerators (InstrumentType::base2_exponential_histogram / kBase2ExponentialHistogram) that don't exist, breaking the build. Also reformats sdk_builder_test.cc per clang-format-18 (CI Format check).
lalitb
left a comment
lalitb
left a comment
There was a problem hiding this comment.
Thanks for fixing. LGTM, you would have to fix the CI build before it can go through.
| { | ||
| case opentelemetry::sdk::metrics::AggregationType::kHistogram: | ||
| sdk_aggregation_config = | ||
| std::make_shared<opentelemetry::sdk::metrics::HistogramAggregationConfig>( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
| EXPECT_NE(aggregation_config, nullptr); | ||
| if (aggregation_config) | ||
| { | ||
| EXPECT_EQ(aggregation_config->GetType(), metrics_sdk::AggregationType::kHistogram); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…mit-only views ThomsonTan pointed out that the HistogramAggregationConfig synthesized in AddView()'s cardinality-limit fallback path default-constructs with empty boundaries_. LongHistogramAggregation/DoubleHistogramAggregation treat a non-null config's boundaries_ literally rather than falling back to the SDK defaults (that fallback only triggers on a null config pointer), so the view registered correctly but collapsed every histogram to a single bucket instead of the SDK's 15-boundary/16-bucket default. Adds HistogramAggregationConfig::DefaultBoundaries() as the single source of truth for those boundaries and uses it in the three places that previously duplicated the literal (both histogram_aggregation.cc constructors, and now sdk_builder.cc). Also strengthens AddViewHistogramCardinalityLimitOnly to build the aggregation via DefaultAggregation::CreateAggregation() and assert on the resulting HistogramPointData's boundary/bucket counts, so the test pins actual behavior rather than just the config's declared type.
|
I've resolved the comments @ThomsonTan please go through them once. |
Both iwyu jobs (all-options-abiv1-preview, all-options-abiv2-preview) flagged sdk_builder_test.cc after the histogram cardinality-limit test additions: missing direct includes for size_t, nostd::function_ref (the FindViews lambda parameter type), and Aggregation (returned by DefaultAggregation::CreateAggregation() and used via ->ToPoint()); and a redundant metric_data.h include already provided transitively through aggregation.h.
The misspell check failed on the previous push due to a transient network error (curl: Recv failure: Connection reset by peer) while downloading the misspell tool itself, before any files were scanned. No misspellings found locally against this exact commit.
5 participants
Summary
Follow-up to #4188, addressing review feedback left after that PR merged
(#4188 (review) and inline comments on
sdk_builder.cc/metric_reader.h).When a view's stream configuration set
aggregation_cardinality_limitwithout an explicitaggregationblock,SdkBuilder::AddView()always constructed a plainAggregationConfig. For instruments whose default aggregation iskHistogramorkBase2ExponentialHistogram, this doesn't match the typeViewRegistry::AddView()requires, so the whole view was silently rejected (AggregationType and AggregationConfig type mismatch. Ignoring AddView call.) and the configured cardinality limit never took effect.AddView()now resolves the instrument's effective aggregation type (falling back to the instrument-derived default the same wayViewRegistry::AddView()does) and constructs the matchingAggregationConfigsubclass (HistogramAggregationConfig/Base2ExponentialHistogramAggregationConfig/ plainAggregationConfig) before applying the limit.sdk/src/configuration/sdk_builder.cc: fixAddView()to pick the correctAggregationConfigsubtype.sdk/test/configuration/sdk_builder_test.cc: addsSdkBuilder.AddViewHistogramCardinalityLimitOnly(reproduces and verifies the fix; fails against the pre-fix code with the exact "type mismatch" rejection) andSdkBuilder.AddViewCounterCardinalityLimitOnly(guards the still-correct non-histogram path).Reader-level cardinality limit enforcement remains a separate follow-up, as noted in the existing
metric_reader.hTODO — this PR only addresses the histogram view-rejection bug.Test plan
SdkBuilder.AddViewHistogramCardinalityLimitOnlyandSdkBuilder.AddViewCounterCardinalityLimitOnlypass.AddViewHistogramCardinalityLimitOnlyfails against the pre-fix code with the reportedViewRegistry::AddViewrejection.yaml_metrics_test,configured_sdk_test,view_registry_test, and the fullsdk_builder_testsuite — all green.