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

overwrite span id when attribute set, query unique traces #155

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Nov 5, 2024

Important

Ensure unique trace results and allow parent span ID overriding based on attributes in trace.rs and spans.rs.

  • Behavior:
    • In get_traces() in trace.rs, added DISTINCT ON (start_time, id) to ensure unique trace results.
    • Modified ORDER BY clause in get_traces() to include id for consistent ordering.
    • In from_otel_span() in spans.rs, set parent_span_id to None if OVERRIDE_PARENT_SPAN_ATTRIBUTE_NAME is true.
  • Constants:
    • Added OVERRIDE_PARENT_SPAN_ATTRIBUTE_NAME in spans.rs to manage parent span ID overriding.
  • Misc:
    • Updated should_keep_attribute() in spans.rs to exclude OVERRIDE_PARENT_SPAN_ATTRIBUTE_NAME.

This description was created by Ellipsis for 5258fea. 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 5258fea in 1 minute and 0 seconds

More details
  • Looked at 61 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. app-server/src/traces/spans.rs:318
  • Draft comment:
    The logic to set parent_span_id to None when OVERRIDE_PARENT_SPAN_ATTRIBUTE_NAME is true is correct and consistent with the intended behavior.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The OVERRIDE_PARENT_SPAN_ATTRIBUTE_NAME is correctly used to set parent_span_id to None when the attribute is present and true. This logic is consistent with the intended behavior described in the comments.

Workflow ID: wflow_KYk0nkmPVa2jkwtN


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.

@@ -435,7 +436,7 @@ pub async fn get_traces(
add_filters_to_traces_query(&mut query, &filters);

query
.push(" ORDER BY start_time DESC OFFSET ")
.push(" ORDER BY start_time DESC, id OFFSET ")
Copy link
Contributor

Choose a reason for hiding this comment

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

The ORDER BY clause should match the columns in the DISTINCT ON clause to ensure consistent results. Consider ordering by start_time, id instead of start_time DESC, id.

Suggested change
.push(" ORDER BY start_time DESC, id OFFSET ")
.push(" ORDER BY start_time, id OFFSET ")

@dinmukhamedm dinmukhamedm merged commit 77637c6 into dev Nov 5, 2024
1 check passed
@dinmukhamedm dinmukhamedm deleted the overwrite-parent branch November 5, 2024 03:09
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