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

Middleware auth, traces page efficiency #184

Merged
merged 4 commits into from
Nov 9, 2024
Merged

Middleware auth, traces page efficiency #184

merged 4 commits into from
Nov 9, 2024

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Nov 9, 2024

  • Add project auth to middleware (for now edge calls node to check)
  • Don't join spans on the count spans query
  • Another slight optimization on spans query
  • Initial link between spans and datapoints
  • various UX improvements

Important

This pull request enhances middleware authentication, optimizes database schema and queries, and improves frontend UX and performance.

  • Middleware:
    • Added project-based authentication in middleware.ts using a POST request to /api/auth.
    • Redirects unauthorized users to /not-found.
  • Database:
    • Created datapoint_to_span table with foreign keys to dataset_datapoints and spans.
    • Added indexes to spans and traces tables for performance optimization.
    • Updated labels table to set value column as NOT NULL.
  • Frontend:
    • Removed isCurrentUserMemberOfProject checks from multiple API routes.
    • Added SpanDatasets component to display datasets linked to a span.
    • Enhanced DataTable component with row selection and pagination features.
    • Improved SessionsTable and TracesTable with better filtering and pagination.
    • Various UI enhancements in Dataset, ExportSpansDialog, and TraceView.

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

More details
  • Looked at 4251 lines of code in 37 files
  • Skipped 1 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/lib/db/migrations/schema.ts:525
  • Draft comment:
    Ensure that the primary key definition here matches the SQL migration file to maintain consistency between the schema and the database.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The datapointToSpan table in the schema file has a primary key defined on three columns: datapointId, spanId, and projectId. This matches the SQL migration file, ensuring consistency between the schema and the database.
2. frontend/lib/db/migrations/0004_worthless_kitty_pryde.sql:6
  • Draft comment:
    Ensure that the primary key definition here matches the schema file to maintain consistency between the schema and the database.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The datapointToSpan table in the SQL migration file has a primary key defined on three columns: datapoint_id, span_id, and project_id. This matches the schema file, ensuring consistency between the schema and the database.

Workflow ID: wflow_2TFF4k6LQq9SPwt3


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.

method: 'POST',
body: JSON.stringify({ projectId }),
headers: {
...Object.fromEntries(req.headers),
Copy link
Contributor

Choose a reason for hiding this comment

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

Forwarding all request headers can lead to security vulnerabilities. Consider explicitly setting only the necessary headers for the internal request.

Suggested change
...Object.fromEntries(req.headers),
'Content-Type': 'application/json',

@dinmukhamedm dinmukhamedm merged commit 00ab4ef into main Nov 9, 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