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: update default alert ch queries for traces and logs #6552

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

nityanandagohain
Copy link
Member

@nityanandagohain nityanandagohain commented Nov 28, 2024

Part of https://github.com/SigNoz/engineering-pod/issues/2036

  • Added ts_bucket_start filter to both the queries.
  • for traces update the table name the attributes key

Also @srikanthccv is peer.service is something we want to keep in this query ?


Important

Update default alert queries in defaults.ts by adding ts_bucket_start filter and modifying trace query table name and attribute key.

  • Behavior:
    • Add ts_bucket_start filter to log and trace queries in defaults.ts.
    • Update table name to signoz_traces.distributed_signoz_index_v3 and attribute key to resource_string_service$$name for trace queries.
  • Misc:
    • Update query in logAlertDefaults to include ts_bucket_start filter.
    • Update query in traceAlertDefaults to include ts_bucket_start filter and change table name and attribute key.

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

@github-actions github-actions bot added the bug Something isn't working label Nov 28, 2024
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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 7291250 in 20 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/container/CreateAlertRule/defaults.ts:120
  • Draft comment:
    Ensure that the subtraction of 1800 from {{.start_timestamp}} is intentional and correct. Consider adding a comment to explain its purpose.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR updates the query to include a filter on ts_bucket_start, which is a good addition for performance. However, the subtraction of 1800 from {{.start_timestamp}} should be clarified or documented to ensure it's intentional and correct.
2. frontend/src/container/CreateAlertRule/defaults.ts:152
  • Draft comment:
    Ensure that the subtraction of 1800 from {{.start_timestamp}} is intentional and correct. Consider adding a comment to explain its purpose.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR updates the query to include a filter on ts_bucket_start, which is a good addition for performance. However, the subtraction of 1800 from {{.start_timestamp}} should be clarified or documented to ensure it's intentional and correct.
3. frontend/src/container/CreateAlertRule/defaults.ts:120
  • Draft comment:
    Avoid hardcoding color values in SQL queries. Use design tokens or predefined color constants for consistency.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_u9LIOh5hAhEPxCHF


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

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@srikanthccv
Copy link
Member

let's change it to use service.name

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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! Incremental review on 645a660 in 25 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/container/CreateAlertRule/defaults.ts:152
  • Draft comment:
    Ensure that replacing attributes_string['peer.service'] with resource_string_service$$name is intentional and aligns with the intended logic. The PR description questions the necessity of peer.service, so double-check if this change is correct.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The query in the traceAlertDefaults object has been updated to use resource_string_service$$name instead of attributes_string['peer.service']. This change aligns with the PR description, which mentions updating the table name and attributes key. However, the PR description also questions whether peer.service should be retained. This suggests a potential oversight or misunderstanding about the necessity of peer.service. Since the PR author has not removed it, it might be intentional, but it's worth confirming if this change aligns with the intended logic.
2. frontend/src/container/CreateAlertRule/defaults.ts:152
  • Draft comment:
    Avoid hardcoding color values in SQL queries. Use design tokens or predefined color constants for consistency.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_nUU95OsH1X11MgAZ


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

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Do we have docs somewhere explaining the bucket timestamp? If not, please create them and link the docs in the query comments

@@ -149,7 +149,7 @@ export const traceAlertDefaults: AlertDef = {
chQueries: {
A: {
name: 'A',
query: `SELECT \n\ttoStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, \n\tstringTagMap['peer.service'] AS op_name, \n\ttoFloat64(avg(durationNano)) AS value \nFROM signoz_traces.distributed_signoz_index_v2 \nWHERE stringTagMap['peer.service']!='' \nAND timestamp BETWEEN {{.start_datetime}} AND {{.end_datetime}} \nGROUP BY (op_name, interval);\n\n-- available variables:\n-- \t{{.start_datetime}}\n-- \t{{.end_datetime}}\n\n-- required column alias:\n-- \tvalue\n-- \tinterval`,
query: `SELECT \n\ttoStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, \n\tresource_string_service$$name AS op_name, \n\ttoFloat64(avg(duration_nano)) AS value \nFROM signoz_traces.distributed_signoz_index_v3 \nWHERE resource_string_service$$name !='' \nAND timestamp BETWEEN {{.start_datetime}} AND {{.end_datetime}} \nAND ts_bucket_start BETWEEN {{.start_timestamp}} - 1800 AND {{.end_timestamp}} \nGROUP BY (op_name, interval);\n\n-- available variables:\n-- \t{{.start_datetime}}\n-- \t{{.end_datetime}}\n-- \t{{.start_timestamp}}\n-- \t{{.end_timestamp}}\n\n-- required column alias:\n-- \tvalue\n-- \tinterval`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
query: `SELECT \n\ttoStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, \n\tresource_string_service$$name AS op_name, \n\ttoFloat64(avg(duration_nano)) AS value \nFROM signoz_traces.distributed_signoz_index_v3 \nWHERE resource_string_service$$name !='' \nAND timestamp BETWEEN {{.start_datetime}} AND {{.end_datetime}} \nAND ts_bucket_start BETWEEN {{.start_timestamp}} - 1800 AND {{.end_timestamp}} \nGROUP BY (op_name, interval);\n\n-- available variables:\n-- \t{{.start_datetime}}\n-- \t{{.end_datetime}}\n-- \t{{.start_timestamp}}\n-- \t{{.end_timestamp}}\n\n-- required column alias:\n-- \tvalue\n-- \tinterval`,
query: `SELECT \n\ttoStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, \n\tresource_string_service$$name AS `service.name`, \n\ttoFloat64(avg(duration_nano)) AS value \nFROM signoz_traces.distributed_signoz_index_v3 \nWHERE resource_string_service$$name !='' \nAND timestamp BETWEEN {{.start_datetime}} AND {{.end_datetime}} \nAND ts_bucket_start BETWEEN {{.start_timestamp}} - 1800 AND {{.end_timestamp}} \nGROUP BY (`service.name`, interval);\n\n-- available variables:\n-- \t{{.start_datetime}}\n-- \t{{.end_datetime}}\n-- \t{{.start_timestamp}}\n-- \t{{.end_timestamp}}\n\n-- required column alias:\n-- \tvalue\n-- \tinterval`,

@nityanandagohain
Copy link
Member Author

Will have to update the docs for clickhouse queries all together which is a sepearete task, will do that and get back to this. Will hold this pr till then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants