fix(otel): sync OTEL_METRICS_KNOWN_FIELD_LIST with emitted keys (#1723) - #1724
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe OTEL known-field list now matches emitted metric keys. Histogram min/max and summary quantile values are excluded from series identity. Top-level ChangesOTEL series identity
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@parmesant @nikhilsinhaparseable Please review this issue and fix! |
|
@prabhaks can you address the conflict pls |
…eablehq#1723) compute_series_hash treats any top-level data-point key not in OTEL_METRICS_KNOWN_FIELD_LIST as a series label, so the list must match the keys the flatteners actually emit. It had drifted both ways: - Emitted but missing (fragmented series): `min` and `max` (histogram per-sample stats) and `data_point_quantile_values` (summary quantile array). Their per-sample values leaked into __series_hash, splitting a physical series into many. - Listed but never emitted as top-level keys (dead entries): `quantile` and `value` (only emitted nested inside data_point_quantile_values), plus the never-used data_point_quantile_values_quantile / data_point_quantile_values_value. `value`/`quantile` also risked collisions: a user attribute of that name was silently dropped from series identity, merging distinct series. Add the 3 missing keys, remove the 4 dead ones (length 37 -> 36), and add tests for min/max and quantile-value fragmentation plus the value/quantile collision.
5c90805 to
cb8d8cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/otel/metrics.rs (1)
207-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep legacy flat exemplar attributes out of
__series_hash.With
flatten_exemplarsenabled,insert_attributes(map, &exemplar(filtered_attributes))writes non-prefixed exemplar keys into the data-point map.compute_series_hashtreats every top-level key outsideOTEL_METRICS_KNOWN_FIELDSand withoutexemplars_as a label, so traces or spans with different exemplar attributes are hashed as different physical series. Excludefiltered_attributesfrom the hash, or prefix these flat keys so only the legacyexemplar_*columns are emitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otel/metrics.rs` around lines 207 - 228, Update the flatten_exemplars handling around insert_attributes and compute_series_hash so filtered exemplar attributes cannot contribute to __series_hash. Either exclude those attributes when computing the hash or prefix their flattened keys consistently, while preserving emission of the legacy exemplar_* columns and ensuring only actual series labels affect the hash.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/otel/metrics.rs`:
- Around line 66-68: Update the histogram field handling in the metrics
serialization and known-field definitions so per-sample statistics use
non-conflicting keys such as data_point_min and data_point_max. Ensure emitted
histogram.min and histogram.max values and the corresponding
OTEL_METRICS_KNOWN_FIELD_LIST entries use the prefixed names consistently,
preserving user min/max attributes for __series_hash.
---
Outside diff comments:
In `@src/otel/metrics.rs`:
- Around line 207-228: Update the flatten_exemplars handling around
insert_attributes and compute_series_hash so filtered exemplar attributes cannot
contribute to __series_hash. Either exclude those attributes when computing the
hash or prefix their flattened keys consistently, while preserving emission of
the legacy exemplar_* columns and ensuring only actual series labels affect the
hash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73faa5d3-fe22-42ee-9743-ef4d30175996
📒 Files selected for processing (1)
src/otel/metrics.rs
…ibutes Bare min/max top-level keys collide with any OTel attribute literally named min or max once added to OTEL_METRICS_KNOWN_FIELD_LIST, wrongly excluding that attribute from the series hash. Renamed to data_point_min/data_point_max, matching every other data-point field's naming convention, and added a regression test for the collision case.
|
Good catch, thanks. Bare min/max would have shadowed a real user attribute with the same name and wrongly excluded it from the series hash, the same class of bug this PR is meant to fix for value/quantile. Renamed the emitted keys to data_point_min/data_point_max, matching the existing naming convention for every other data point field (data_point_value, data_point_count, data_point_sum, etc), and added a regression test that a user attribute literally named min or max still affects the series hash. Pushed in e417c87. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/otel/metrics.rs (1)
367-368: 🗄️ Data Integrity & Integration | 🔵 TrivialRenaming histogram
min/max→data_point_min/data_point_maxis a breaking change for existing consumers.This correctly fixes the shadowing issue raised in the previous review, but note that any dashboards, saved queries, or downstream tooling referencing the old bare
min/maxhistogram columns will silently stop populating after this change (and historical rows retain the old column names). Worth a release/migration note if such consumers exist.🔍 Verification script
#!/bin/bash # Look for other references to bare histogram min/max fields that might need updating. rg -nP --type=rust '"min"|"max"' src/ --glob '!src/otel/metrics.rs' rg -n 'histogram.*min|histogram.*max' docs/ 2>/dev/null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otel/metrics.rs` around lines 367 - 368, Document the breaking rename performed in the histogram serialization around insert_number_if_some, including migration guidance for consumers of the former bare min/max columns and the new data_point_min/data_point_max names. Preserve the current field names in code and add the release or migration note in the project’s established documentation location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/otel/metrics.rs`:
- Around line 367-368: Document the breaking rename performed in the histogram
serialization around insert_number_if_some, including migration guidance for
consumers of the former bare min/max columns and the new
data_point_min/data_point_max names. Preserve the current field names in code
and add the release or migration note in the project’s established documentation
location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56d25bd2-01ca-47d7-b67a-0b460a99ffed
📒 Files selected for processing (1)
src/otel/metrics.rs
|
@parmesant whenever you get time - please look into this! |
Signed-off-by: parmesant <anant.v09@protonmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/otel/metrics.rs (1)
42-42: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix the known-field array length.
OTEL_METRICS_KNOWN_FIELD_LISTdeclares[&str; 38], but the initializer contains 37 entries. Rust rejects the mismatch at compile time, and the same list length assertion expects 37.Proposed fix
-pub const OTEL_METRICS_KNOWN_FIELD_LIST: [&str; 38] = [ +pub const OTEL_METRICS_KNOWN_FIELD_LIST: [&str; 37] = [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otel/metrics.rs` at line 42, Update the array length declaration in OTEL_METRICS_KNOWN_FIELD_LIST from [&str; 38] to [&str; 37] to match the number of string entries actually provided in the initializer, which contains 37 elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/otel/metrics.rs`:
- Line 42: Update the array length declaration in OTEL_METRICS_KNOWN_FIELD_LIST
from [&str; 38] to [&str; 37] to match the number of string entries actually
provided in the initializer, which contains 37 elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c9370c88-f319-4466-b965-8a396314c0da
📒 Files selected for processing (1)
src/otel/metrics.rs
it still appears in list of columns for entries where it can't be flattened
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/otel/metrics.rs (1)
1112-1135: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix the known-field validation test before merge.
At Line 1112, the test expects 37 entries, but the list contains 39 entries. After that correction, Lines 1130-1131 also fail because those fields are intentionally present in the known-field list.
Proposed fix
- assert_eq!(OTEL_METRICS_KNOWN_FIELD_LIST.len(), 37); + assert_eq!(OTEL_METRICS_KNOWN_FIELD_LIST.len(), 39); ... - "data_point_quantile_values_quantile", - "data_point_quantile_values_value",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otel/metrics.rs` around lines 1112 - 1135, Update the known-field validation test around OTEL_METRICS_KNOWN_FIELD_LIST to expect 39 entries instead of 37. Revise the negative assertions for the intentionally present fields so they no longer require absence, while preserving absence checks only for fields that were actually removed from OTEL_METRICS_KNOWN_FIELDS.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/otel/metrics.rs`:
- Around line 1112-1135: Update the known-field validation test around
OTEL_METRICS_KNOWN_FIELD_LIST to expect 39 entries instead of 37. Revise the
negative assertions for the intentionally present fields so they no longer
require absence, while preserving absence checks only for fields that were
actually removed from OTEL_METRICS_KNOWN_FIELDS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f725bb6-38cd-404d-854d-0c0c395fdb6c
📒 Files selected for processing (1)
src/otel/metrics.rs
4 participants
What
Fixes #1723.
compute_series_hashinsrc/otel/metrics.rstreats every top-level data-point key that is not inOTEL_METRICS_KNOWN_FIELD_LISTas a series label. The list had drifted from the keys the flatteners actually emit, in both directions, corrupting physical-series identity for metrics.Emitted but missing from the list (series fragmentation)
These are emitted as top-level keys but were absent from the list, so their per-sample values were hashed into
__series_hash, splitting one physical series into many:min,max— histogram per-sample statistics (flatten_histogram)data_point_quantile_values— summary quantile array (flatten_summary)Listed but never emitted top-level (dead entries, with collision risk)
value,quantile— only ever emitted nested inside thedata_point_quantile_valuesobjects, never as top-level keys. Because they were listed, a user attribute literally namedvalueorquantilewas silently excluded from series identity, merging distinct series.valueis a common label name, so this was the higher-risk one.data_point_quantile_values_quantile,data_point_quantile_values_value— never emitted anywhere. Harmless dead entries, removed for correctness.Change
data_point_quantile_valuesto the list.value,quantile,data_point_quantile_values_quantile,data_point_quantile_values_value.min/maxtodata_point_min/data_point_max, matching the naming convention every other data-point field already uses (data_point_value,data_point_count,data_point_sum, etc.), and add both to the list under their new names. This was raised in review: baremin/maxwould have shadowed a real user attribute of the same name and wrongly excluded it from the series hash, the same collision risk asvalue/quantileabove.mainpost-fix(otel): preserve all exemplars per data point (#1662) #1720, which already carriesexemplars).data_point_min/data_point_max(histogram) anddata_point_quantile_values(summary) no longer changecompute_series_hash; a data-point attribute namedvalue,quantile,min, ormaxstill does; plus a guard asserting the list contents and length.Verification
cargo test --lib otel::metrics: 18 passed, 0 failed.cargo clippy --all-targets -- -D warnings: clean on this file.mainafter fix(otel): preserve all exemplars per data point (#1662) #1720 merged.Summary by CodeRabbit
Bug Fixes
Tests