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

write labels to clickhouse #204

Merged
merged 1 commit into from
Nov 14, 2024
Merged

write labels to clickhouse #204

merged 1 commit into from
Nov 14, 2024

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Nov 14, 2024

  • Write label values to clickhouse whenever we write them to Postgres
  • Similarly delete label values from clickhouse when deleted from Postgres
  • Fix adding a label with 0 value key (frontend)
  • Fix frontend warning about <p> in <DialogDescription> which internally is a <p> as well

Important

This PR adds functionality to write and delete label values in Clickhouse alongside Postgres, fixes frontend label validation, and addresses a UI warning.

  • Behavior:
    • Write label values to Clickhouse in insert_or_update_label() in labels/mod.rs and record_labels_to_db() in traces/utils.rs.
    • Delete label values from Clickhouse in delete_span_label() in routes/labels.rs.
    • Fix frontend validation for label value existence in add-label-popover.tsx.
    • Fix frontend warning about nested <p> tags in evaluator-editor-dialog.tsx.
  • Database:
    • Add labels table to Clickhouse in 001000-initial.sql.
  • Misc:
    • Remove unused import in datasets.rs.

This description was created by Ellipsis for 3f62a28. 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.

👍 Looks good to me! Reviewed everything up to 3f62a28 in 1 minute and 25 seconds

More details
  • Looked at 745 lines of code in 17 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. app-server/src/ch/labels.rs:13
  • Draft comment:
    Implement From<LabelSource> for u8 instead of Into<u8> for better clarity and practice.
impl From<LabelSource> for u8 {
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The Into<u8> implementation for LabelSource should be replaced with From<LabelSource> for u8 for better practice and clarity.
2. app-server/src/ch/labels.rs:86
  • Draft comment:
    Remove the dbg! macro as it is used for debugging purposes and should not be present in production code.
    // dbg!(&label);
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. app-server/src/ch/labels.rs:120
  • Draft comment:
    Consider adding a semicolon at the end of the SQL query for consistency and to follow common SQL practices.
        .query("DELETE FROM labels WHERE project_id = ? AND span_id = ? AND id = ?;")
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The SQL query in delete_label function lacks a semicolon at the end, which is a common practice in SQL statements.
4. app-server/src/routes/labels.rs:185
  • Draft comment:
    Consider handling the case where the label does not exist in Clickhouse after deletion from Postgres to avoid potential errors.
    if let Err(e) = crate::ch::labels::delete_label(clickhouse.clone(), project_id, span_id, label_id).await {
        log::error!("Failed to delete label from Clickhouse: {:?}", e);
    }
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative, suggesting a potential issue without evidence of an actual problem. The code already attempts to delete the label from Clickhouse, and any error handling should be part of the existing logic. Without evidence of an error occurring, the comment does not indicate a clear code change requirement.
    There might be an edge case where the label does not exist in Clickhouse, leading to an error. However, without evidence of this causing issues, the comment remains speculative.
    The comment does not provide evidence of an actual issue occurring, and the code already handles deletion. Speculative comments are not useful without evidence.
    Delete the comment as it is speculative and does not provide strong evidence of an issue that requires a code change.
5. app-server/src/ch/labels.rs:125
  • Draft comment:
    Consider handling the case where the label does not exist in Clickhouse after deletion from Postgres to avoid potential errors.
    if let Err(e) = client
        .query("DELETE FROM labels WHERE project_id = ? AND span_id = ? AND id = ?")
        .bind(project_id)
        .bind(span_id)
        .bind(id)
        .execute()
        .await {
        log::error!("Failed to delete label from Clickhouse: {:?}", e);
    }
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_ugxFbe0AtGpJgygf


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@dinmukhamedm dinmukhamedm merged commit fa8370e into dev Nov 14, 2024
1 check passed
@dinmukhamedm dinmukhamedm deleted the labels-ch branch November 14, 2024 06:23
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