[Metrics SDK] Enforce MetricReader-level cardinality limits as a fallback during collection - #4388
Conversation
…back during collection Fixes open-telemetry#4387 Follow-up to open-telemetry#4188 and open-telemetry#4314. Reader-level cardinality limits were parsed and stored on MetricReader but never enforced. The initial attempt to enforce them in open-telemetry#4188 by resolving the reader's limit directly into shared per-view storage was reverted during review since it broke per-reader semantics: with two readers sharing one instrument's storage, a stricter reader would let the laxer reader lose data, or a laxer reader would force every reader down to its limit. This enforces limits at both ends instead: - Shared recording storage (SyncMetricStorage/AsyncMetricStorage) is now sized at the max cardinality limit across all attached readers when the view has no explicit limit, so no reader loses data. - Each reader's own (possibly stricter) limit is re-applied to just its own output during collection (TemporalMetricStorage:: buildMetrics), via AttributesHashMap's existing overflow mechanism. View-level limits still take precedence over reader-level ones, unchanged, per the View > Reader > SDK default spec precedence.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4388 +/- ##
==========================================
+ Coverage 82.63% 82.67% +0.04%
==========================================
Files 512 512
Lines 20138 20193 +55
==========================================
+ Hits 16639 16692 +53
- Misses 3499 3501 +2
🚀 New features to boost your workflow:
|
…c no longer uses aggregation_config.h directly
lalitb
left a comment
lalitb
left a comment
There was a problem hiding this comment.
Thanks for working on this. The overall approach makes sense, but I found a few cases where metric values can be lost or the configured reader limit is not applied. I think these should be fixed before merging, so I am requesting changes.
…s view-explicit detection Addresses three issues @lalitb found in review: 1. AttributesHashMap::Set() overwrote the existing overflow aggregation instead of merging into it. With several distinct attribute sets routed to overflow (as now happens routinely with a low reader-level limit), only the last one survived, silently dropping the others. Fixed by merging via Aggregation::Merge(), matching the existing GetOrSetDefault() overflow behavior. 2. ResolveRecordingCardinalityLimit() floored the recording storage size at kAggregationCardinalityLimit (2000) even when every attached reader's own limit was lower, so a single delta reader with e.g. limit=3 could still see up to 2000 series via the single-collector delta fast path in TemporalMetricStorage::buildMetrics(). Fixed by removing the floor: the max is now taken purely across attached readers' configured limits (falling back to the SDK default only when there are no readers yet). 3. A non-null AggregationConfig doesn't necessarily mean a view explicitly set a cardinality limit: SdkBuilder::AddView() may build one purely to carry histogram boundaries, leaving cardinality_limit_ at its compiled-in default. Treating that as "explicit" silently skipped the MetricReader-level fallback. Added AggregationConfig::cardinality_limit_explicit_ (defaults to true, preserving existing programmatic/test behavior) and have AddView() set it false when a config is synthesized without aggregation_cardinality_limit, true when it is set. Tests added: overflow-merge total-value assertion on the existing multi-reader test, a new single-delta-reader fast-path regression test, and two SdkBuilder tests pinning cardinality_limit_explicit_ for the histogram-boundaries-only and boundaries-plus-limit cases.
@lalitb Thank you for thorough review, I've addressed the requested changes, please go through them once. |
…rdinality-limit-fallback # Conflicts: # CHANGELOG.md # sdk/src/configuration/sdk_builder.cc # sdk/test/configuration/sdk_builder_test.cc
|
@lalitb I've resolved the merge conflicts as well, updating with main. |
… delegating ctors still used the old name The exemplar-filters refactor (open-telemetry#4267) that landed on main renamed the long-standing typo'd parameter exempler_filter_type to exemplar_filter_type in some but not all of the lines my branch also touched, so git's line-based auto-merge produced no conflict marker but left the two delegating-constructor overloads in sync_metric_storage.h/async_metric_storage.h referencing the old name in their forwarding call / parameter declaration while the member-init list used the new one. Only builds with ENABLE_METRICS_EXEMPLAR_PREVIEW defined actually instantiate that code path, which is why local default-config builds didn't catch it before pushing. Verified with a throwaway build with WITH_METRICS_EXEMPLAR_PREVIEW=ON (opentelemetry_metrics, metric_collector_test, metric_reader_test, sync/async_instruments_test, async_metric_storage_test all build and pass), and re-verified the default (flag off) build still passes.
…rdinality-limit-fallback # Conflicts: # CHANGELOG.md
…rdinality-limit-fallback # Conflicts: # CHANGELOG.md
…rdinality-limit-fallback # Conflicts: # CHANGELOG.md
|
@lalitb small reminder on this, I've rebased the branch so no merge conflicts anymore. |
Thanks for addressing the earlier findings. I found two remaining cases where the reader-level limit is not applied correctly. I think these should be fixed before merging. |
…tic configs, document late-reader recording-capacity limitation Per lalitb's review: - AggregationConfig/HistogramAggregationConfig/ Base2ExponentialHistogramAggregationConfig previously defaulted cardinality_limit_explicit_ to true unconditionally, so a programmatically-constructed config built only for some other reason (e.g. HistogramAggregationConfig() to set histogram boundaries) was wrongly treated as having set an explicit cardinality limit, silently shadowing the MetricReader-level fallback. Added a sentinel (kCardinalityLimitUnspecified) so the constructor can tell apart "no cardinality_limit argument was given" from "cardinality_limit was explicitly passed", without needing <optional> (avoided elsewhere in the SDK for ABI reasons). This is correct for every construction path - tests, programmatic API, and SdkBuilder - without each caller having to set the flag manually. - Recording capacity for an instrument is resolved once, as the highest limit across readers attached at instrument-creation time, and is not grown retroactively. A reader added later with a higher limit cannot recover attribute sets that already collapsed into overflow before it was attached. Documented this on MeterContext::AddMetricReader and MeterProvider::AddMetricReader, and added a regression test pinning the behavior.
@lalitb thanks for the thorough review. I've pushed a fix for the two remaining findings: Please check once more! |
| @@ -40,13 +52,21 @@ class AggregationConfig | |||
| } | |||
|
|
|||
| size_t cardinality_limit_; | |||
There was a problem hiding this comment.
This public field can still be assigned directly, but that no longer marks the limit as explicit.
For example:
HistogramAggregationConfig config;
config.cardinality_limit_ = 100;cardinality_limit_explicit_ remains false, so the reader fallback can override the configured limit. Could we keep these two values from getting out of sync, or preserve the existing direct-assignment behavior another way?
There was a problem hiding this comment.
Fixed. Made both fields private with a SetCardinalityLimit() setter that sets them together; direct assignment is no longer possible.
| // without affecting what other collectors of the same storage see. | ||
| const bool has_explicit_view_limit = | ||
| aggregation_config_ && aggregation_config_->cardinality_limit_explicit_; | ||
| std::unique_ptr<AttributesHashMap> merged_metrics(new AttributesHashMap( |
There was a problem hiding this comment.
This applies the reader limit only in the normal merge path. The single-reader delta fast path above returns delta_metrics directly, before reaching this code.
For example, if storage was created with capacity 2000 and a delta reader with limit 3 is attached later, that fast path can still emit more than 3 attribute sets.
Could we apply the reader fallback in the delta fast path too, and add a regression test for a stricter reader attached after instrument creation?
There was a problem hiding this comment.
Fixed. The fast path now re-caps through a hashmap sized to the collector's effective limit when the recorded size exceeds it, mirroring the merge path below. Added a test that fails without this fix (10 points, no overflow, instead of capped at 3).
| // hashmap to maintain the metrics for delta collection (i.e, collection since last Collect call) | ||
| const AggregationConfig *aggregation_config_; | ||
| // Capacity used to (re)size attributes_hashmap_. See the constructor comment above. | ||
| const std::size_t recording_cardinality_limit_; |
There was a problem hiding this comment.
The bound-instrument admission path does not use this resolved recording limit. ResolveCardinality() still uses aggregation_config_->cardinality_limit_.
These values can now differ when the limit comes from a reader. For example, with a reader limit of 3 and no explicit view limit, bound keys are admitted using 2000 even though the recording hashmap is capped at 3.
Could we use recording_cardinality_limit_ in ResolveCardinality() so bound and unbound recording use the same effective limit?
There was a problem hiding this comment.
Fixed. ResolveCardinality() now reads recording_cardinality_limit_ instead of the aggregation config's own limit, so bound and unbound admission always agree.
|
@om7057 - Thanks, I checked the latest update. The original programmatic-constructor issue is fixed. I am also okay with treating a higher limit from a late-added reader as a documented limitation, since existing instrument storage is not rebuilt dynamically. During the recheck, I found three related cases in the new implementation: direct assignment to the public limit field, the single-reader delta fast path, and bound-instrument admission. I have left separate inline comments for them. Cardinality-limit handling crosses recording storage, per-reader collection, overflow, and optional bound-instrument paths, so small changes can affect several code paths. I think changes in this area need careful review and focused regression coverage. Once those are addressed, I will check it again. |
…path re-cap, fix bound-instrument admission limit Three findings from the recheck of the earlier fixes: - AggregationConfig::cardinality_limit_ was still a public field, so direct assignment (config.cardinality_limit_ = 100) could desync it from cardinality_limit_explicit_, silently letting a MetricReader fallback override a value the caller thought was set explicitly. Made both fields private; added SetCardinalityLimit() to set them together, and GetCardinalityLimit()/IsCardinalityLimitExplicit() accessors. Updated every call site (meter.cc, temporal_metric_storage.cc, sync/async storage headers, sdk_builder.cc, sdk_builder_test.cc); the manual cardinality_limit_explicit_ = false workaround in SdkBuilder::AddView() is no longer needed since AggregationConfig's constructor now derives it correctly for every construction path. - The single-collector delta fast path in TemporalMetricStorage::buildMetrics() returned the raw recording storage directly, bypassing the per-collector re-cap. When recording capacity was resolved larger than this reader's own limit (e.g. the reader was attached after the instrument already existed, so its limit was never part of the max-across-readers resolution at creation time), the fast path could emit more attribute sets than the reader's configured limit. Now re-caps through a hashmap sized to the collector's effective limit before emitting, when the recorded size exceeds it. - SyncMetricStorage::ResolveCardinality() (bound-instrument admission, preview) used aggregation_config_->GetCardinalityLimit() instead of recording_cardinality_limit_. These can differ when the limit comes from a MetricReader fallback rather than an explicit view limit, since attributes_hashmap_ (the unbound path) is sized to recording_cardinality_limit_. Now uses the same resolved limit so bound and unbound admission agree. Also fixed a pre-existing test bug found while verifying the second fix: ReaderCardinalityLimitFallbackSingleDeltaReader used MockMetricReader's default exporter, which reports kCumulative, not kDelta, so it never actually exercised the single-collector-delta fast path its name and comment claimed to test. Fixed to construct an explicit kDelta exporter, and added ReaderCardinalityLimitFallbackSingleDeltaReaderAttachedLate, which does exercise that path and fails without the fix (verified by temporarily reverting it locally: 10 points emitted with no overflow instead of capped at the reader's limit of 3).
@lalitb all three addressed: AggregationConfig fields are now private behind SetCardinalityLimit(), the delta fast path re-caps to the collector's limit, and bound/unbound admission both use recording_cardinality_limit_. Replied inline on each with specifics. Added a regression test for the fast-path bug that I verified fails without the fix. Ready for another look whenever you have time. |
…r_test.cc needs push_metric_exporter.h temporal_metric_storage.cc's new effective_limit/AttributesHashMap recap logic uses std::size_t directly. metric_collector_test.cc now constructs MockMetricExporter/PushMetricExporter directly instead of only through common.h.
…rdinality-limit-fallback # Conflicts: # CHANGELOG.md
2 participants
Summary
Fixes #4387
Follow-up to #4188 and #4314.
MetricReader::SetCardinalityLimits()parsed and stored reader-level cardinality limits but never enforced them (see the TODO that was inMetricReader::SetCardinalityLimits()). The initial attempt to enforce them in #4188 resolved the reader's limit directly into shared per-view storage, which was reverted during review because it broke per-reader semantics: with two readers sharing one instrument's storage at different limits, a reader could either lose data it should have gotten, or force every other reader down to its own stricter limit.Changes
This enforces the View > Reader > SDK default precedence at both ends of the pipeline:
SyncMetricStorage/AsyncMetricStorage): when a view has no explicitAggregationConfigcardinality limit, the sharedAttributesHashMapused for recording is now sized at the max cardinality limit across allMetricReaders currently attached, so no reader loses data purely because storage was capped too low for it. When a view does set an explicit limit, storage sizing is unchanged.TemporalMetricStorage::buildMetrics): each reader's own (possibly stricter) limit is re-applied to just its own collected output, viaAttributesHashMap's existing overflow mechanism (Set()already redirects new keys beyond capacity into the overflow point). This means a stricter reader is still capped to what it configured, without affecting other readers sharing the same storage.SyncMetricStorage/AsyncMetricStoragegained a new constructor parameter (recording_cardinality_limit) for this; the original constructor signature is preserved as a delegating overload so any external caller is unaffected.Tests
MetricCollectorTest.ReaderCardinalityLimitFallbackWithMultipleReaders: two readers (limits 3 and 50, no view-level limit) share one counter; the low-limit reader is capped with overflow present, the high-limit reader sees every recorded series with no data loss and no overflow. Confirmed this test fails against the pre-fix code (low-limit reader saw all 10 points, no overflow).metric_reader_test,metric_collector_test,sync_instruments_test,async_instruments_test,async_metric_storage_test,bound_sync_instruments_testunderWITH_METRICS_BOUND_INSTRUMENTS_PREVIEW) passes unchanged.Checklist
CHANGELOG.mdupdated