-
-
Notifications
You must be signed in to change notification settings - Fork 153
update: field names of attributes in each metric type #1456
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.
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
update: field names of attributes in each metric type #1456
Conversation
add prefix `metric_` in the attribute names
WalkthroughThe changes refactor attribute handling in OpenTelemetry metrics by introducing namespaced attribute insertion. Metric attributes are now prefixed with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/otel/metrics.rs (2)
27-31: Consider grouping imports from the same module.The
flatten_attributesimport on line 27 could be combined with the otherotel_utilsimports on lines 29-31 for better organization.Apply this diff:
-use crate::otel::otel_utils::flatten_attributes; - use super::otel_utils::{ - convert_epoch_nano_to_timestamp, insert_attributes, insert_number_if_some, + convert_epoch_nano_to_timestamp, flatten_attributes, insert_attributes, insert_number_if_some, };
654-666: Add documentation for the new helper functions.These helper functions introduce an important behavioral change (prefixing attribute keys), which should be documented to explain their purpose and the prefixing convention.
Example documentation:
/// Inserts metric data point attributes into the map with a "metric_" prefix. /// This namespaces metric attributes to avoid conflicts with other fields. fn insert_metric_attributes(map: &mut Map<String, Value>, attributes: &[KeyValue]) { let attributes_json = flatten_attributes(attributes); for (key, value) in attributes_json { map.insert(format!("metric_{}", key), value); } } /// Inserts exemplar attributes into the map with an "exemplar_" prefix. /// This namespaces exemplar attributes to avoid conflicts with other fields. fn insert_exemplar_attributes(map: &mut Map<String, Value>, attributes: &[KeyValue]) { let attributes_json = flatten_attributes(attributes); for (key, value) in attributes_json { map.insert(format!("exemplar_{}", key), value); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/otel/metrics.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/otel/metrics.rs (1)
src/otel/otel_utils.rs (1)
flatten_attributes(152-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
🔇 Additional comments (3)
src/otel/metrics.rs (3)
79-79: LGTM: Consistent application of attribute prefixing.The new helper functions are consistently applied across all data point and exemplar attribute insertion points, correctly implementing the namespacing strategy described in the PR objectives.
Also applies to: 126-126, 224-224, 321-321, 390-390
494-494: Verify whether selective attribute prefixing aligns with the PR intent ("add prefixmetric_in the attribute names").The PR updated data point attributes (lines 126, 224, 321, 390) and exemplar attributes (line 79) to use
insert_metric_attributes()andinsert_exemplar_attributes()respectively, which addmetric_andexemplar_prefixes. However, the following remain unprefixed via the genericinsert_attributes():
- Line 494: metric metadata attributes
- Line 525: resource attributes
- Line 550: scope attributes
The PR commit message is ambiguous about whether prefixing should apply to all attributes or only data point-level attributes. Confirm this selective approach is intentional and aligned with the intended OTEL metrics schema.
33-68: The concern aboutOTEL_METRICS_KNOWN_FIELD_LISTfiltering or rejecting prefixed dynamic attributes is unfounded. The known_fields list is used exclusively for metadata storage inLogSourceEntryand stream schema tracking—it is never passed to or consulted during actual data processing. Theflatten_and_push_logsand related processing functions that handle incoming data do not reference this list, so prefixed attributes likemetric_<attr_name>andexemplar_<attr_name>will be processed normally without restriction.Likely an incorrect or invalid review comment.
add prefix
metric_in the attribute namesSummary by CodeRabbit