fix(otel): sync OTEL_METRICS_KNOWN_FIELD_LIST with emitted keys (#1723)#1724
fix(otel): sync OTEL_METRICS_KNOWN_FIELD_LIST with emitted keys (#1723)#1724prabhaks wants to merge 1 commit into
Conversation
…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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe OTEL known-field list was synchronized with emitted metric keys. Histogram ChangesOTEL series identity
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
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
min,max,data_point_quantile_valuesto the list.value,quantile,data_point_quantile_values_quantile,data_point_quantile_values_value.min/max(histogram) anddata_point_quantile_values(summary) no longer changecompute_series_hash; a data-point attribute namedvalueorquantilenow does; plus a guard asserting the list contents and length.Verification
cargo test --lib otel::metrics: 10 passed, 0 failed.cargo clippy --no-deps -- -D warnings: clean.Note
min/maxare emitted with bare (un-prefixed) names, unlike the otherdata_point_*stats. This PR only syncs the known-field list with what is currently emitted. Renaming them todata_point_min/data_point_maxfor consistency and to remove the (smaller) collision surface would be a separate, output-schema-changing follow-up worth discussing.Summary by CodeRabbit
valueorquantileare now preserved correctly when used as labels.