fix: stringify otel attributes of array types #1298
Conversation
|
Warning Rate limit exceeded@nikhilsinhaparseable has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce a shared mutable map for "other_attributes" that is propagated through flattening functions across OpenTelemetry metrics, logs, and traces modules. This map accumulates attributes from resource, scope, and record levels and is serialized as a JSON string into each flattened output record. In the utilities module, attribute processing is refactored to recursively convert nested array and key-value list types into JSON values, separating known attributes by prefix from others. The HTTP cluster handler closure is simplified for conciseness without changing behavior. These modifications enhance attribute collection and representation consistency across telemetry data. Changes
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/otel/metrics.rs (1)
209-221: Improved serialization of explicit_bounds arraysSimilar to the bucket_counts change, this properly serializes explicit_bounds arrays as strings. This approach is consistent and helps ensure compatibility with various JSON consumers.
However, there's duplicated code pattern between this and the bucket_counts implementation. Consider extracting a helper function:
+ fn array_to_json_string<T>(values: &[T], value_converter: impl Fn(&T) -> Value) -> String { + let array = Value::Array(values.iter().map(value_converter).collect()); + serde_json::to_string(&array).unwrap() + } // Then use it: - let data_point_bucket_counts = Value::Array( - data_point - .bucket_counts - .iter() - .map(|&count| Value::Number(count.into())) - .collect(), - ); - let data_point_bucket_counts_string = - serde_json::to_string(&data_point_bucket_counts).unwrap(); + let data_point_bucket_counts_string = array_to_json_string( + &data_point.bucket_counts, + |&count| Value::Number(count.into()) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/otel/metrics.rs(1 hunks)src/otel/otel_utils.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
🔇 Additional comments (6)
src/otel/metrics.rs (1)
196-208: Improved serialization of bucket_counts arraysThe code now properly stringifies the bucket_counts array values, which aligns with the PR objective to fix OTEL attributes of array types. This change prevents potential issues with large arrays or systems that might not handle nested arrays well.
src/otel/otel_utils.rs (5)
21-22: LGTM: Updated imports for new functionalityAdding imports for
ArrayValueandKeyValueListis appropriate for the new helper functions.
43-49: Improved array handling with proper stringificationGood refactoring to handle array values. Converting arrays to JSON strings ensures consistency with the PR objective and prevents issues with complex nested structures.
50-56: Improved key-value list handlingThe refactored approach for handling key-value lists is cleaner and more maintainable. Using a dedicated helper function improves code organization.
68-100: Well-structured recursive array handlingThe new helper function handles all value types appropriately and properly processes nested structures. This is a good implementation that ensures consistent serialization of array values.
102-112: Effective key-value list processingThis helper function correctly processes key-value lists and integrates well with the existing code. The recursive approach ensures proper handling of nested structures.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/otel/logs.rs (1)
60-64: Update doc‑comment to mentionother_attributesparameterThe rust‑doc above still says the function “returns a
Map” but does not describe the new argument or its purpose. Readers will miss the side‑effect of mutating the shared map.src/otel/traces.rs (1)
50-52: Doc comment drift – add note aboutother_attributesparameter
flatten_scope_span’s documentation hasn’t been updated to reflect the additional argument and its side‑effects. Please amend for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/otel/logs.rs(9 hunks)src/otel/metrics.rs(16 hunks)src/otel/otel_utils.rs(5 hunks)src/otel/traces.rs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/otel/otel_utils.rs
- src/otel/metrics.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/otel/traces.rs (1)
src/otel/otel_utils.rs (1)
insert_attributes(182-191)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (2)
src/otel/logs.rs (1)
153-163: Globalother_attributesmap could overwrite per‑record dataBecause every call to
insert_attributesmutates the sameother_attributesmap, attributes with identical keys but different values coming from distinct resources / scopes / records will overwrite one another. The final string therefore reflects only the last occurrence of each key, which may not be what you intend.Can you confirm that “last‑writer wins” is acceptable? If not, consider collecting values per record or storing conflicting values in an array.
src/otel/traces.rs (1)
95-104: Potential key‑collision overwrite in sharedother_attributesAll resource‑level, scope‑level and span‑level “other” attributes funnel into a single map. Any duplicate keys will be overwritten. Verify that data loss is acceptable; otherwise accumulate per level or convert to
Vec<(String, Value)>to preserve duplicates.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/otel/metrics.rs (1)
552-564:other_attributescloned for every record – avoid redundant allocation
other_attributesis serialised once but the resultingStringis cloned into every metric record, which may be thousands of times per OTEL message.-let other_attributes = match serde_json::to_string(&other_attributes) { +let other_attributes_str = match serde_json::to_string(&other_attributes) { … }; -for metric_record_json in &mut vec_otel_json { - metric_record_json.insert( - "other_attributes".to_string(), - Value::String(other_attributes.clone()), - ); +let other_attributes_value = Value::String(other_attributes_str); +for metric_record_json in &mut vec_otel_json { + metric_record_json + .insert("other_attributes".to_string(), other_attributes_value.clone()); }Cloning a
Value(which holds anArc) is cheaper than cloning a largeStringN times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/otel/logs.rs(8 hunks)src/otel/metrics.rs(16 hunks)src/otel/otel_utils.rs(5 hunks)src/otel/traces.rs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/otel/logs.rs
- src/otel/otel_utils.rs
- src/otel/traces.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
🔇 Additional comments (2)
src/otel/metrics.rs (2)
91-104: Mutation of a sharedother_attributesmap can cause key collisionsThe same mutable map is threaded through every attribute‑insertion call.
If different resources / scopes / data‑points contain the same attribute key with different values, the later value silently overwrites the former.This makes the final
"other_attributes"field order‑dependent and may hide valuable context.Consider namespacing by level (e.g.
resource.foo,scope.bar) or accumulating conflicting values into an array instead of overwriting.Would you like a concrete proposal or a small proof‑of‑concept patch to detect collisions during insertion?
431-456: 👍 Addedmetric_typeimproves downstream queryingIncluding the metric’s concrete variant (
gauge,sum, …) as a first‑class field simplifies analytics and is forward‑compatible. Nice touch!
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/otel/metrics.rs (1)
70-74:⚠️ Potential issue
unwrap()onfrom_f64may panic for NaN / ±∞ values
serde_json::Number::from_f64(*double_val).unwrap()will panic when OTEL sends non‑finite numbers, which are valid in the protobuf model.-Value::Number(serde_json::Number::from_f64(*double_val).unwrap()) +Value::Number( + serde_json::Number::from_f64(*double_val) + .unwrap_or_else(|| { + tracing::warn!("non‑finite exemplar value encountered, serialising as null"); + serde_json::Number::from(0) // or return Value::Null + }) +)Consider returning
Value::Null(or a string) instead of crashing the pipeline.
♻️ Duplicate comments (2)
src/otel/metrics.rs (2)
42-83:⚠️ Potential issueExemplar loop still overwrites previous exemplars – data loss risk remains
The loop keeps mutating a single
exemplar_jsonmap, so only the last exemplar survives.
A prior review already pointed this out but the logic is unchanged; the newly‑addedother_attributesparameter doesn’t address the core problem. Returning a collection (e.g.Vec<Map<…>>) or prefixing keys with the exemplar index is required to preserve all exemplars.-fn flatten_exemplar( - exemplars: &[Exemplar], - other_attributes: &mut Map<String, Value>, -) -> Map<String, Value> { - let mut exemplar_json = Map::new(); - for exemplar in exemplars { - ... - } - exemplar_json -} +fn flatten_exemplar( + exemplars: &[Exemplar], + other_attributes: &mut Map<String, Value>, +) -> Vec<Map<String, Value>> { + exemplars + .iter() + .map(|exemplar| { + let mut exemplar_json = Map::new(); + insert_attributes(&mut exemplar_json, &exemplar.filtered_attributes, other_attributes); + /* copy remaining inserts … */ + exemplar_json + }) + .collect() +}Call‑sites would then merge/flatten this vector appropriately.
Without the change, exemplar information is silently dropped.
221-236:⚠️ Potential issueHistogram explicit‑bounds may panic for non‑finite values – issue persists
Same
unwrap()risk as above; the prior review suggested graceful handling but the code is unchanged.-Value::Number(serde_json::Number::from_f64(*bound).unwrap()) +serde_json::Number::from_f64(*bound) + .map(Value::Number) + .unwrap_or(Value::Null)This avoids runtime panics when OTEL sends
NaN,+∞, or-∞.
🧹 Nitpick comments (1)
src/otel/metrics.rs (1)
91-103: Sharedother_attributesmap can clobber values across data‑points / resourcesBecause the same mutable map is threaded through all records, unknown attributes with identical keys but different values will overwrite one another, leaving only the last occurrence. This can blur attribute provenance and produce misleading output.
Options:
- Reset the map per metric record (or per resource) before passing it down.
- Change
other_attributestoMap<String, Vec<Value>>so you can keep all observed values.- Attach
other_attributesdirectly to the record where they originate instead of a global merge.Please verify that the current aggregation behaviour is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/otel/logs.rs(8 hunks)src/otel/metrics.rs(16 hunks)src/otel/traces.rs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/otel/logs.rs
- src/otel/traces.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/otel/metrics.rs (1)
506-541: 💡 Verification agent🧩 Analysis chain
other_attributesis accumulated across all resource/scope/metric records – may cause key collisionA single mutable map is reused for the whole
MetricsDatamessage.
If different resources reuse the same attribute key with different values, the later value clobbers the former, and the final"other_attributes"string no longer represents any single record accurately.Consider:
- Resetting
other_attributesper top‑levelresource_metricsblock (movelet mut other_attributes = Map::new();inside that loop), or- Storing a vector of attribute maps keyed by resource/scope to keep provenance.
🏁 Script executed:
grep -R "let mut other_attributes" -n src/otel/metrics.rsLength of output: 106
🏁 Script executed:
grep -R "fn insert_attributes" -n src/otelLength of output: 97
🏁 Script executed:
grep -R -n -C5 "other_attributes" src/otel/metrics.rsLength of output: 11939
Move
other_attributesinitialization into the resource loop to avoid cross‐record contaminationCurrently in
src/otel/metrics.rsat line 506 you have:pub fn flatten_otel_metrics(message: MetricsData) -> Vec<Value> { let mut vec_otel_json = Vec::new(); let mut other_attributes = Map::new(); for record in &message.resource_metrics { … } // serialize & attach `other_attributes` for all records }Because that single
other_attributesmap is shared across every resource/scope/metric,
– attributes from an earlier resource get mixed into a later one
– duplicate keys overwrite previous values
– the final"other_attributes"no longer accurately reflects any one recordSuggested fix:
- Remove the top‑level
let mut other_attributes = Map::new();- Add
let mut other_attributes = Map::new();immediately inside thefor record in &message.resource_metrics { … }blockExample diff:
pub fn flatten_otel_metrics(message: MetricsData) -> Vec<Value> { let mut vec_otel_json = Vec::new(); - let mut other_attributes = Map::new(); for record in &message.resource_metrics { + let mut other_attributes = Map::new(); // … same logic, now scoped per-record } // adjust serialization/attachment to use the per-record map }This change ensures each resource’s attributes are collected and stringified in isolation, preventing key collisions and cross‑record bleed.
♻️ Duplicate comments (1)
src/otel/metrics.rs (1)
568-581: Serialisingother_attributesas a duplicated JSON string is still problematicPrevious reviews already highlighted that turning a structured map into a huge string field inflates every record and forces consumers to re‑parse JSON.
The code remains unchanged, so the concern stands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/otel/metrics.rs(17 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/otel/metrics.rs (1)
src/otel/otel_utils.rs (2)
insert_attributes(182-191)convert_epoch_nano_to_timestamp(193-196)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (1)
src/otel/metrics.rs (1)
228-247: Good defensive handling of f64 → JSON conversionSwitching from
unwrap()tomap(...).unwrap_or(Value::Null)prevents panics on NaN/∞ bounds – nice catch.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/otel/traces.rs (1)
346-361: String formatting could be improved for attribute concatenation.When concatenating attributes from different sources, the current approach might lead to formatting inconsistencies if any of the strings are empty or already contain delimiters.
Consider using a more robust approach for combining attribute strings:
- span_record_json.insert( - "other_attributes".to_string(), - Value::String(format!( - "{other_attributes} {events_other_attributes} {links_other_attributes}" - )), - ); + let mut combined = Vec::new(); + if !other_attributes.is_empty() { + combined.push(other_attributes); + } + if !events_other_attributes.is_empty() { + combined.push(events_other_attributes); + } + if !links_other_attributes.is_empty() { + combined.push(links_other_attributes); + } + span_record_json.insert( + "other_attributes".to_string(), + Value::String(combined.join(", ")), + );This approach handles empty strings better and ensures consistent delimiter usage.
src/otel/otel_utils.rs (2)
45-51: Improved array value handling with stringification.The recursive approach for handling array values is good, but there's a potential issue with error handling.
The
unwrap()call on line 48 could fail if serialization encounters an error. Consider adding error handling:- let json_array_string = serde_json::to_string(&json_array_value).unwrap(); + let json_array_string = match serde_json::to_string(&json_array_value) { + Ok(s) => s, + Err(e) => { + tracing::warn!("failed to serialise array value: {e}"); + String::default() + } + };
206-213: String concatenation could be improved.The concatenation in the
merge_attributes_in_jsonfunction might lead to formatting inconsistencies.Consider using a more robust approach for combining attribute strings:
- let other_attributes = format!("{other_attributes}, {attributes}"); + // Check if other_attributes is empty or already ends with a comma + let other_attributes = if other_attributes.is_empty() { + attributes.clone() + } else if other_attributes.trim_end().ends_with(',') { + format!("{other_attributes} {attributes}") + } else { + format!("{other_attributes}, {attributes}") + };This handles edge cases better when combining attribute strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/otel/logs.rs(7 hunks)src/otel/otel_utils.rs(6 hunks)src/otel/traces.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/otel/logs.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (18)
src/otel/traces.rs (9)
28-31: New imports support attribute handling enhancements.The added imports bring in the utility functions needed for the new attribute handling functionality, which is a good preparation for the changes that follow.
52-55: Good implementation of attribute collection and merging.The use of a mutable
other_attributesmap for collecting non-standard attributes is a clean pattern. Themerge_attributes_in_jsoncall at line 91 correctly applies these attributes back to the scope span JSON.Also applies to: 90-91
67-71: Updated insert_attributes call with other_attributes parameter.The function call has been properly updated to match the new signature, passing the mutable
other_attributesmap for collecting attributes that don't match known prefixes.
99-100: Similar attribute handling pattern for resource spans.Consistent with the implementation in
flatten_scope_span, using the same pattern for collection and merging of attributes. This consistency helps maintainability.Also applies to: 132-134
105-109: Updated insert_attributes call matches new function signature.The function call has been correctly updated to pass the mutable
other_attributesmap.
147-148: Event attributes properly serialized to string.The implementation correctly collects other attributes and serializes them to a string. The empty check before insertion prevents unnecessary string operations.
Also applies to: 156-156, 161-168
181-182: Links attributes handling matches event pattern.The implementation for links follows the same pattern as events, maintaining consistency in the codebase.
Also applies to: 192-192, 197-204
288-290: Span record properly collects attributes.The
other_attributesmap is initialized and passed toinsert_attributescorrectly for the span record.Also applies to: 325-329
334-337: Fetching nested attributes from events and links.Good implementation that extracts attributes from nested structures, ensuring all attributes are captured.
Also applies to: 342-345
src/otel/otel_utils.rs (9)
20-22: Good constant definition for known attribute prefixes.Adding imports for the required types and defining a constant for known attribute prefixes is a clean approach that centralizes the configuration.
Also applies to: 25-26
52-58: Refactored key-value list processing.The refactoring for key-value list processing is cleaner and more maintainable than the previous implementation.
70-102: Well-implemented recursive array value processing.The function handles all possible value types in the array, including nested arrays and key-value lists, ensuring complete conversion to JSON.
104-114: Good implementation of key-value list processing.The function correctly processes all key-value pairs in the list, recursively handling values through
collect_json_from_value.
139-160: Well-designed attribute separation by prefix.The function now properly separates attributes into known and other categories based on their prefix, which is a good design for the filtering requirement.
182-191: Updated insert_attributes with simplified implementation.The function has been correctly updated to pass the
other_attributes_jsontoflatten_attributes.
198-222: New utility for merging attributes into JSON.The function effectively merges attributes into JSON objects, handling cases where the key already exists by concatenating values.
224-234: Utility for aggregating attribute strings.Good implementation for fetching and combining attribute strings from JSON maps.
236-244: Good error handling for attribute serialization.The function properly handles serialization errors by logging a warning and returning a default string, ensuring the operation doesn't fail.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/otel/otel_utils.rs (3)
76-108: Well-structured recursive function for array value conversion.The new helper function properly handles all OTEL value types and recursively processes nested structures. Consider adding documentation comments to explain the purpose and behavior of this function.
+/// Recursively converts an OpenTelemetry ArrayValue into a JSON Value +/// +/// This handles nested array values and key-value lists by recursively +/// converting them to their JSON representations. fn collect_json_from_array_value(array_value: ArrayValue) -> Value {
110-123: Good error handling in key-value list processing.The function correctly handles missing values with proper error logging instead of unwrapping, which addresses the previous review comment about unwrap calls. Consider adding documentation comments here as well.
+/// Recursively converts an OpenTelemetry KeyValueList into a JSON Map +/// +/// This handles missing values gracefully and logs warnings instead of panicking. fn collect_json_from_key_value_list(key_value_list: KeyValueList) -> Map<String, Value> {
207-231: Good implementation of attribute merging with error handling.The new function handles the concatenation of attribute data efficiently and includes appropriate error handling. There's a potential issue with unwrapping, but you've mitigated this with
unwrap_or_default().Consider using pattern matching to make the code more robust:
- if json.contains_key("other_attributes") { - let other_attributes = json.get_mut("other_attributes").unwrap(); - let other_attributes = other_attributes.as_str().unwrap_or_default(); - // append the other_attributes to the scope log json - let other_attributes = format!("{other_attributes} {attributes}"); - json.insert( - "other_attributes".to_string(), - Value::String(other_attributes), - ); + if let Some(existing) = json.get_mut("other_attributes") { + if let Some(existing_str) = existing.as_str() { + // append the other_attributes to the scope log json + *existing = Value::String(format!("{existing_str} {attributes}")); + } else { + // Existing value is not a string, replace it + *existing = Value::String(attributes.clone()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/otel/metrics.rs(20 hunks)src/otel/otel_utils.rs(6 hunks)src/otel/traces.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/otel/traces.rs
- src/otel/metrics.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
🔇 Additional comments (8)
src/otel/otel_utils.rs (8)
20-22: Import additions look good.Added imports for
ArrayValueandKeyValueListtypes are necessary for the new recursive helper functions and improve code readability by making dependencies explicit.
25-26: Good use of constant for attribute classification.The new
KNOWN_ATTRIBUTES_PREFIXconstant improves maintainability by centralizing the list of attribute prefixes considered "known". This makes it easier to update the list in the future without searching through the code.
45-57: Improved array value handling with proper error handling.The refactored code now properly handles array values by converting them to JSON strings with appropriate error handling. This is a good approach for preserving complex nested array structures.
58-64: Better key-value list handling with dedicated helper function.Moving key-value list processing to a dedicated helper function improves code organization and makes the logic easier to maintain.
148-169: Well-implemented attribute separation logic.The modified function now properly separates attributes into "known" and "other" categories based on their key prefixes. This approach allows for more flexible attribute handling downstream.
191-200: Updated function signature for consistent attribute handling.The
insert_attributesfunction now accepts and passes theother_attributes_jsonmap to maintain consistency with the updatedflatten_attributesfunction. This ensures uniform attribute handling across the codebase.
233-243: Effective attribute extraction function.This function efficiently extracts and combines attribute data from multiple JSON maps. The approach is clean and straightforward.
245-253: Well-handled attribute serialization with proper error logging.The function includes appropriate error handling and logging if serialization fails. This prevents runtime issues and makes debugging easier.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/otel/otel_utils.rs (2)
25-26: Consider documenting the purpose of the known attribute prefixes.Adding a brief comment explaining why these specific prefixes are considered "known" would improve maintainability for future developers.
-const KNOWN_ATTRIBUTES_PREFIX: [&str; 6] = ["http", "url", "service", "os", "host", "telemetry"]; +/// Prefixes of attribute keys that should be preserved as top-level fields in flattened output. +/// Other attributes will be collected in a separate JSON object under "other_attributes". +const KNOWN_ATTRIBUTES_PREFIX: [&str; 6] = ["http", "url", "service", "os", "host", "telemetry"];
249-262: Consider handling non-stringother_attributesvalues.The function only handles
other_attributesvalues that are strings, but themerge_attributes_in_jsonfunction suggests they could also be objects. Consider adding support for object values as well for consistency.pub fn fetch_attributes_from_json(json_arr: &Vec<Map<String, Value>>) -> Map<String, Value> { let mut merged_attributes = Map::new(); for json in json_arr { - if let Some(Value::String(attrs_str)) = json.get("other_attributes") { - if let Ok(attrs) = serde_json::from_str::<Map<String, Value>>(attrs_str) { + if let Some(attrs_val) = json.get("other_attributes") { + let attrs = match attrs_val { + Value::String(attrs_str) => serde_json::from_str::<Map<String, Value>>(attrs_str) + .unwrap_or_else(|e| { + tracing::warn!("failed to parse other_attributes string: {e}"); + Map::new() + }), + Value::Object(obj) => obj.clone(), + _ => Map::new() + }; for (key, value) in attrs { merged_attributes.insert(key, value); } - } } } merged_attributes }src/otel/traces.rs (1)
162-168: Consider consolidating serialization pattern across files.The pattern of checking if other_attributes is empty, serializing, and adding to the JSON is repeated in both event and link handling. Consider extracting this to a helper function in otel_utils.rs for consistency.
// Add to otel_utils.rs pub fn add_other_attributes_if_not_empty( json: &mut Map<String, Value>, other_attributes: &Map<String, Value> ) { if !other_attributes.is_empty() { let attrs_str = fetch_attributes_string(other_attributes); json.insert("other_attributes".to_string(), Value::String(attrs_str)); } } // Then in this code and similar places: -if !other_attributes.is_empty() { - let other_attributes = fetch_attributes_string(&other_attributes); - event_json.insert( - "other_attributes".to_string(), - Value::String(other_attributes), - ); -} +add_other_attributes_if_not_empty(&mut event_json, &other_attributes);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/otel/metrics.rs(21 hunks)src/otel/otel_utils.rs(6 hunks)src/otel/traces.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/otel/metrics.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/otel/traces.rs (1)
src/otel/otel_utils.rs (5)
fetch_attributes_from_json(249-262)fetch_attributes_string(267-275)insert_attributes(191-200)merge_attributes_in_json(210-244)convert_epoch_nano_to_timestamp(202-205)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (27)
src/otel/otel_utils.rs (10)
20-22: Good addition of required imports.These imports are needed for the new functionality that handles
ArrayValueandKeyValueListtypes more efficiently.
45-57: Good implementation for array value serialization.This properly handles array values by recursively converting them to JSON and then stringifying to ensure consistency in the output format. The error handling is appropriate.
58-64: Good implementation for key-value list handling.Using the new helper function to handle key-value lists and then merging the results into the value JSON map is a clean approach.
76-108: Well-implemented recursive array value conversion.The function properly handles all possible value types within arrays, including nested arrays and key-value lists. The null-safety with
if let Some(val)is good practice.
110-123: Improved error handling for key-value list processing.The implementation now properly handles the case where a key-value pair might not have a value, avoiding the potential for unwrap panics that was present in an earlier version (as mentioned in the past review comments).
148-151: Updated function signature to support attribute segregation.The addition of the
other_attributes_jsonparameter allows for effective separation of known and unknown attributes.
158-165: Well-implemented attribute categorization logic.The code now correctly categorizes attributes based on the defined prefixes, storing them either in the main map or the "other_attributes" map.
191-200: Updated insert_attributes to work with separated attributes.This change aligns well with the updated
flatten_attributesfunction to properly separate and handle attributes.
210-244: Complex merge logic deserves unit testing.The
merge_attributes_in_jsonfunction has multiple code paths for different scenarios, which makes it a good candidate for unit testing to ensure all edge cases are handled correctly.Consider adding unit tests for the various scenarios:
- When
other_attributesalready exists as a string- When
other_attributesexists as an object- When
other_attributesdoesn't exist- When serialization fails at different steps
267-275: Good error handling in fetch_attributes_string.The function properly handles serialization errors and logs them with an appropriate warning.
src/otel/traces.rs (17)
28-31: Good addition of required imports.These imports bring in the necessary utility functions for handling and merging attribute maps.
55-55: Good initialization of other_attributes map.This establishes the pattern of collecting non-standard attributes separately.
67-71: Updated insert_attributes call with other_attributes.The function call is properly updated to pass the other_attributes map.
90-91: Effective merging of other_attributes.Using merge_attributes_in_json to add other_attributes to all scope span JSON objects is a clean approach.
100-100: Good initialization of top-level other_attributes map.Starting with an empty map at the resource level allows for attribute accumulation across the entire trace.
105-109: Updated resource insert_attributes call.The function call is properly updated to include other_attributes.
132-133: Effective merging of other_attributes at resource level.This ensures all resource-level attributes are properly merged into each resource span.
148-148: Good pattern for event-level other_attributes.Creating a separate map for each event follows the established pattern.
156-156: Updated event insert_attributes call.The function call is properly updated to include other_attributes.
182-182: Good pattern for link-level other_attributes.Creating a separate map for each link follows the established pattern.
192-192: Updated link insert_attributes call.The function call is properly updated to include other_attributes.
198-204: Same serialization pattern as with events.This follows the same pattern established for events, maintaining consistency.
289-289: Good initialization of span-level other_attributes.This establishes the pattern at the span record level.
325-329: Updated span insert_attributes call.The function call is properly updated to include other_attributes.
334-337: Good extraction of events with other_attributes.The code properly flattens events and extracts their other_attributes for later merging.
342-345: Good extraction of links with other_attributes.The code properly flattens links and extracts their other_attributes for later merging.
347-359: Well-implemented attribute merging across levels.The code effectively merges attributes from the span, its events, and its links, providing a comprehensive view of all attributes in the span record.
However, there's some redundancy in the merging code that could be simplified:
-if !other_attributes.is_empty() - || !events_other_attributes.is_empty() - || !links_other_attributes.is_empty() -{ - for (key, value) in &events_other_attributes { - other_attributes.insert(key.clone(), value.clone()); - } - for (key, value) in &links_other_attributes { - other_attributes.insert(key.clone(), value.clone()); - } - let attrs_str = fetch_attributes_string(&other_attributes); - span_record_json.insert("other_attributes".to_string(), Value::String(attrs_str)); -} +// Merge all attributes +other_attributes.extend(events_other_attributes.clone()); +other_attributes.extend(links_other_attributes.clone()); + +if !other_attributes.is_empty() { + let attrs_str = fetch_attributes_string(&other_attributes); + span_record_json.insert("other_attributes".to_string(), Value::String(attrs_str)); +}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/otel/otel_utils.rs (1)
39-46: 🛠️ Refactor suggestionInconsistent numeric representation for
IntValuebetween top‑level and nested handling
IntValueis serialized to a string here, while insidecollect_json_from_array_valueit is serialized to a JSON number.
This discrepancy makes the JSON shape unpredictable for consumers who cannot rely on a stable type.- OtelValue::IntValue(int_val) => { - value_json.insert(key.to_string(), Value::String(int_val.to_string())); - } + OtelValue::IntValue(int_val) => { + value_json.insert( + key.to_string(), + Value::Number(serde_json::Number::from(int_val)), + ); + }Align both code paths to use
Number(preferred) orStringconsistently.src/otel/traces.rs (1)
100-135:⚠️ Potential issue
other_attributesleaks between ResourceSpans – reset it inside the loop
other_attributesis declared once before iterating overmessage.resource_spansand is continuously grown:let mut other_attributes = Map::new(); for record in &message.resource_spans { ... insert_attributes(&mut resource_span_json, &resource.attributes, &mut other_attributes); ... merge_attributes_in_json(other_attributes.clone(), &mut vec_resource_spans_json);As a result, unknown attributes present in earlier resources are merged into later ones, producing incorrect data and potential attribute collisions.
Move the map inside the
for recordloop (or clear it at the start of every iteration):-for record in &message.resource_spans { - let mut resource_span_json = Map::new(); - let mut other_attributes = Map::new(); +for record in &message.resource_spans { + let mut resource_span_json = Map::new(); + let mut other_attributes = Map::new();This confines the accumulation to the current resource span only.
🧹 Nitpick comments (4)
src/otel/otel_utils.rs (3)
88-103: Avoid cloning entire nested arrays while recursing
collect_json_from_array_valuecurrently clones every nestedArrayValuebefore the recursive call:let nested_json = collect_json_from_array_value(arr.clone());For large telemetry payloads this multiplies allocations unnecessarily.
Taking the parameter by reference (and cloning only when you must keep ownership) removes the extra copies:-fn collect_json_from_array_value(array_value: ArrayValue) -> Value { +fn collect_json_from_array_value(array_value: &ArrayValue) -> Value { @@ - OtelValue::ArrayValue(arr) => { - let nested_json = collect_json_from_array_value(arr.clone()); + OtelValue::ArrayValue(arr) => { + let nested_json = collect_json_from_array_value(arr);You would also need to adapt the initial caller:
-let json_array_value = collect_json_from_array_value(array_val); +let json_array_value = collect_json_from_array_value(&array_val);
156-174: Possible key shadowing & needless lookup insideflatten_attributesInside the inner loop the variable
keyhides the outerkeyborrowed from the attribute, andvalue_json[key]indexes the map twice:for key in value_json.keys() { if KNOWN_ATTRIBUTES_PREFIX.iter().any(|prefix| key.starts_with(prefix)) { attributes_json.insert(key.to_owned(), value_json[key].to_owned());Using clearer names and iterating on
(k, v)pairs removes an extra map lookup and avoids confusion:for (attr_key, attr_val) in &value_json { if KNOWN_ATTRIBUTES_PREFIX.iter().any(|p| attr_key.starts_with(p)) { attributes_json.insert(attr_key.clone(), attr_val.clone()); } else { other_attributes_json.insert(attr_key.clone(), attr_val.clone()); } }
258-268:fetch_attributes_from_jsonignores non‑stringother_attributesIf earlier code inserted the field as an in‑place object (which is allowed in
merge_attributes_in_json), this helper will simply skip it.
Handle theValue::Objectvariant as well to ensure no attributes are lost.- if let Some(Value::String(attrs_str)) = json.get("other_attributes") { - if let Ok(attrs) = serde_json::from_str::<Map<String, Value>>(attrs_str) { + if let Some(other_attrs) = json.get("other_attributes") { + if let Value::String(attrs_str) = other_attrs { + if let Ok(attrs) = serde_json::from_str::<Map<String, Value>>(attrs_str) { ... } + } else if let Value::Object(obj) = other_attrs { + merged_attributes.extend(obj.clone()); + } }src/otel/traces.rs (1)
90-93: Minor: avoid cloning large maps when merging
merge_attributes_in_json(other_attributes.clone(), &mut vec_scope_span_json);
Passing a reference is enough – the helper can clone internally if it truly needs ownership.This is a micro‑optimisation but can pay off for high‑cardinality telemetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/otel/logs.rs(7 hunks)src/otel/metrics.rs(21 hunks)src/otel/otel_utils.rs(6 hunks)src/otel/traces.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/otel/logs.rs
- src/otel/metrics.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (1)
src/otel/otel_utils.rs (1)
48-59: Converting arrays to JSON strings may hamper downstream processingThe array is first converted to a
Value::Arrayand then immediately stringified before insertion.
If later stages want to inspect, merge or redact individual elements they must first parse the string back to JSON, defeating the purpose.Consider inserting the array as
Value::Arraydirectly (or make stringification configurable via a flag) unless there is a strong schema‑compatibility reason to keep it as a string.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/otel/otel_utils.rs (2)
78-113: Handle integer conversion edge cases consistently.The function thoroughly processes different value types, but there's an inconsistency in how numeric conversions are handled.
For consistency with how you handle doubles (lines 91-95), consider adding a similar safeguard for integers at line 89:
- OtelValue::IntValue(i) => { - json_array.push(Value::Number(serde_json::Number::from(*i))) - } + OtelValue::IntValue(i) => { + // For consistency with double handling + let number = serde_json::Number::from(*i); + json_array.push(Value::Number(number)); + }While integers won't have the same precision issues as floating-point values, this makes the code more consistent and easier to maintain.
244-255: Add error logging for deserialization failures.When merging attributes, deserialization failures are silently ignored, which could make debugging difficult.
Add error logging to help diagnose issues when existing attributes can't be deserialized:
fn merge_with_existing_attributes( attributes: &Map<String, Value>, attrs_str: &str, ) -> Option<String> { - if let Ok(mut existing_attrs) = serde_json::from_str::<Map<String, Value>>(attrs_str) { + match serde_json::from_str::<Map<String, Value>>(attrs_str) { + Ok(mut existing_attrs) => { for (key, value) in attributes { existing_attrs.insert(key.clone(), value.clone()); } return serde_json::to_string(&existing_attrs).ok(); + } + Err(e) => { + tracing::warn!("failed to deserialize existing attributes: {e}"); + None + } } - None }src/otel/traces.rs (1)
132-134: Unnecessary clone of other_attributes.The
other_attributesmap is cloned but not used again in this function after passing tomerge_attributes_in_json.Remove the unnecessary clone to improve performance:
- merge_attributes_in_json(other_attributes.clone(), &mut vec_resource_spans_json); + merge_attributes_in_json(other_attributes, &mut vec_resource_spans_json);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/otel/otel_utils.rs(6 hunks)src/otel/traces.rs(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (12)
src/otel/otel_utils.rs (7)
20-22: Appropriate imports added for array and key-value handling.The imports now correctly include
ArrayValueandKeyValueListfrom the OpenTelemetry proto, which are essential for the new recursive helper functions.
25-28: Well-structured constants with clear documentation.Good job defining these constants with descriptive comments. The
KNOWN_ATTRIBUTES_PREFIXarray provides clear guidance on which attributes should be preserved as individual fields versus collected in theother_attributesobject.
48-59: Improved array handling with proper error handling.The array serialization approach is robust, with excellent error handling for serialization failures. Converting arrays to JSON strings ensures consistent representation regardless of nested complexity.
61-65: Enhanced key-value list processing for better maintainability.The refactored approach using a dedicated helper function improves code organization and maintainability when handling key-value lists.
115-131: Good error handling for key-value list processing.The nested
if letstatements with proper error logging ensure robust handling of potentially missing or invalid values in key-value lists.
156-177: Effective attribute categorization by prefix.The categorization of attributes into "known" vs. "other" based on prefixes provides a clean way to handle attributes consistently across OpenTelemetry data types.
199-208: Updated function signature with proper handling of other attributes.The
insert_attributesfunction is correctly modified to work with the additional parameter for storing "other attributes".src/otel/traces.rs (5)
27-31: Appropriate imports added for attribute handling utilities.The imports have been correctly updated to include the new utility functions for handling "other attributes".
90-92: Good integration of other_attributes with scope spans.The addition of merging other attributes into scope span JSON objects ensures that attributes are consistently handled and preserved.
162-163: Consistent handling of other_attributes in events.Using
add_other_attributes_if_not_emptyappropriately ensures that other attributes are only added when present, preventing empty objects in the resulting JSON.
192-193: Consistent handling of other_attributes in links.Similar to events, the links handling properly uses
add_other_attributes_if_not_emptyfor consistent attribute handling.
323-346: Comprehensive attribute merging from nested structures.The implementation effectively aggregates attributes from nested events and links, ensuring no information is lost during flattening.
Summary by CodeRabbit