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

Fix realtime and manual spans #221

Merged
merged 4 commits into from
Nov 18, 2024
Merged

Fix realtime and manual spans #221

merged 4 commits into from
Nov 18, 2024

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Nov 18, 2024

Fixes to realtime, manual spans, and update the spans and traces tables


Important

This PR refines the database schema by removing unused columns and updates the handling of spans and traces in the backend, with minor frontend adjustments.

  • Database Changes:
    • Remove version, release, user_id, and success columns from traces table.
    • Remove version column from spans table.
    • Add trace_metadata_gin_idx index to traces table.
  • Backend Code Changes:
    • Update TraceAttributes and SpanAttributes to handle metadata and session IDs.
    • Remove handling of version and success in trace.rs and spans.rs.
    • Update record_span_to_db() in utils.rs to reflect changes in trace attributes.
  • Frontend Adjustments:
    • Remove references to version and success in types.ts.
    • Update Pipeline component to handle isSupabaseEnabled prop.

This description was created by Ellipsis for 13c7b5b. 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 13c7b5b in 1 minute and 56 seconds

More details
  • Looked at 5682 lines of code in 17 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/lib/traces/types.ts:57
  • Draft comment:
    The 'success' field has been removed from the Span type in the code changes, but it is still present in this type definition. Consider removing it to keep the type definition consistent with the code changes.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment seems to be incorrect because the 'success' field is not present in the Span type definition in the new file contents. The comment does not align with the current state of the code.
    I might be missing some context about other parts of the codebase, but based on the provided file contents, the comment does not seem relevant.
    The comment should be evaluated based on the provided file contents, which do not include the 'success' field in the Span type definition.
    The comment is not relevant to the current state of the code and should be deleted.

Workflow ID: wflow_iYrc95X4H4RIm4Vy


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.

@@ -108,7 +104,7 @@ export type Trace = {
topSpanName: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

The 'success' field has been removed from the Trace type in the code changes, but it is still present in this TracePreview type. Consider removing it to maintain consistency with the code changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, TracePreview is a legacy type, and we should perhaps remove it in a separate PR. Many of the types in these lib/*/types.ts are outdated and this requires a cleanup

@dinmukhamedm dinmukhamedm merged commit 2bd68d8 into main Nov 18, 2024
1 check 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.

2 participants