Skip to content
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

parse span path as array (#333) #334

Merged
merged 1 commit into from
Jan 19, 2025
Merged

parse span path as array (#333) #334

merged 1 commit into from
Jan 19, 2025

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Jan 19, 2025

Important

Change span path handling from string to array in SpanAttributes and update related functions for compatibility.

  • Behavior:
    • Change span path handling from String to Vec<String> in SpanAttributes.
    • Update flat_path() to join path array into a string.
    • Modify extend_span_path() to append to path array.
  • Functions:
    • Update from_db_span() in CHSpan to use flat_path().
    • Update inner_process_queue_spans() to use flat_path().
    • Modify create_datapoint() to handle path as array.
  • Misc:
    • Remove unused import serde_json::Value in index.rs.

This description was created by Ellipsis for 587138c. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 587138c in 1 minute and 24 seconds

More details
  • Looked at 136 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. app-server/src/traces/consumer.rs:247
  • Draft comment:
    Using unwrap_or_default() can lead to unexpected behavior if the default value is not appropriate. Consider handling the None case explicitly to ensure the correct default behavior.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. app-server/src/ch/spans.rs:94
  • Draft comment:
    Consider defining a constant for the default string "<null>" to avoid repetition and potential errors. This pattern is used multiple times in the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The unwrap_or with a default string "<null>" is used multiple times. It might be better to define a constant for this default value to avoid repetition and potential errors.
3. app-server/src/ch/spans.rs:93
  • Draft comment:
    Consider defining a constant for the default string "<null>" to avoid repetition and potential errors. This pattern is used multiple times in the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The unwrap_or with a default string "<null>" is used multiple times. It might be better to define a constant for this default value to avoid repetition and potential errors.

Workflow ID: wflow_BeULwbLGDq89qiSu


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

if let Some(Value::String(path)) = span.attributes.get(SPAN_PATH) {
data.insert("path".to_string(), path.to_owned());
if let Some(v) = span.attributes.get(SPAN_PATH) {
data.insert("path".to_string(), v.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using v.to_string() on a serde_json::Value may not produce the expected string representation. Consider using as_str() or handling different types explicitly.

@dinmukhamedm dinmukhamedm merged commit feb3125 into main Jan 19, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant