feat: trace V4 QB #6407
feat: trace V4 QB #6407
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to d0595bd in 1 minute and 2 seconds
More details
- Looked at
1350lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/query-service/constants/constants.go:450
- Draft comment:
TheStaticFieldsTracesmap is defined twice, once here and once inquery_builder.go. Consider defining it in one place to avoid inconsistencies. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new mapStaticFieldsTracesinconstants.gowhich is used inquery_builder.goto check if a key is a static field. This map is used to determine if a key should be returned as is in thegetColumnNamefunction. This is a good approach to handle static fields, but it should be noted that the map is defined twice, once inconstants.goand once inquery_builder.go. This could lead to inconsistencies if the maps are not kept in sync. It would be better to define the map in one place and reuse it.
2. pkg/query-service/app/traces/v4/query_builder.go:1
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
3. pkg/query-service/app/traces/v3/query_builder.go:25
-
Draft comment:
This map is already defined inpkg/query-service/app/logs/v3/query_builder.go. Consider using the existing map to avoid duplication. -
constant
AggregateOperatorToSQLFunc(query_builder.go) -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a potential code quality issue (duplication) which is generally useful feedback. However, the map 'AggregateOperatorToSQLFunc' was not newly introduced in this diff; it was only made public. The comment does not directly relate to a change made in this diff, as the map already existed in the file before the change.
The comment might still be useful for future refactoring, but it does not address a change made in this diff. The rules specify that comments should be about changes made in the diff.
While the comment is about a potential improvement, it does not pertain to a change made in this diff, so it should be removed according to the rules.
Remove the comment because it does not pertain to a change made in this diff, even though it highlights a potential code quality improvement.
Workflow ID: wflow_6wbzX5RuVCox9yT2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
nityanandagohain
requested review from
srikanthccv,
shivanshuraj1333 and
eKuG
|
@nityanandagohain this is big PR. Please do it parts. |
|
I have kept just the two major files. Do suggest to break pr's down function wise ? |
|
These can be independently merged? |
|
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4637dc1 in 44 seconds
More details
- Looked at
39lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/app/traces/v4/query_builder.go:259
- Draft comment:
Removing theidcolumn from the select statement forPanelTypeListmight affect functionality ifidwas required. Verify this change to ensure it doesn't break existing functionality. - Reason this comment was not posted:
Comment did not seem useful.
2. pkg/query-service/app/traces/v4/query_builder.go:56
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This comment also applies to other instances in the file. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_AGtK3eSCmU03c4D3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e8705b3 in 34 seconds
More details
- Looked at
140lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. pkg/query-service/app/traces/v4/query_builder.go:35
- Draft comment:
The functiongetClickHouseTracesColumnTypeis correctly implemented. Ensure that all possibleAttributeKeyTypevalues are covered in the test cases. - Reason this comment was not posted:
Confidence changes required:10%
The functiongetClickHouseTracesColumnTypeis straightforward, but the test case names could be more descriptive. However, the implementation is correct.
2. pkg/query-service/app/traces/v4/query_builder.go:32
- Draft comment:
The change fromNOT has(%s%s, '%s')toNOT mapContains(%s, '%s')forFilterOperatorNotExistsis correct and consistent with the logic forFilterOperatorExists. - Reason this comment was not posted:
Confidence changes required:10%
The change in line 32 oftracesOperatorMappingV3is correct, as it aligns with the logic forFilterOperatorExists.
3. pkg/query-service/app/traces/v4/query_builder.go:107
- Draft comment:
The use ofutils.QuoteEscapedStringForContainsin line 107 is appropriate for handling special characters incontainsoperations. - Reason this comment was not posted:
Confidence changes required:10%
The change in line 107 is correct as it uses a specialized function for escaping strings incontainsoperations.
4. pkg/query-service/app/traces/v4/query_builder.go:103
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Oy9qkzcCkJs3Kfst
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fd9dd52 in 36 seconds
More details
- Looked at
27lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/app/logs/v4/query_builder.go:438
- Draft comment:
The removal of the step calculation for the table panel type might affect existing functionality. Ensure that this change aligns with the intended behavior and does not introduce any regressions. - Reason this comment was not posted:
Confidence changes required:50%
The removal of the step calculation for the table panel type in both logs and traces query builders seems intentional. However, it's important to ensure that this change does not affect any existing functionality or assumptions in the codebase.
2. pkg/query-service/app/traces/v4/query_builder.go:292
- Draft comment:
The removal of the step calculation for the table panel type might affect existing functionality. Ensure that this change aligns with the intended behavior and does not introduce any regressions. - Reason this comment was not posted:
Confidence changes required:50%
The removal of the step calculation for the table panel type in both logs and traces query builders seems intentional. However, it's important to ensure that this change does not affect any existing functionality or assumptions in the codebase.
Workflow ID: wflow_vCJf6JkxLkYW7iYE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 789aa0d in 40 seconds
More details
- Looked at
195lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/app/traces/v4/query_builder.go:52
- Draft comment:
The check for static fields is redundant as it is performed twice. Consider moving the static fields check after the column check to avoid redundancy. - Reason this comment was not posted:
Comment was on unchanged code.
2. pkg/query-service/app/traces/v4/query_builder.go:50
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_VsK5G3KX7eXY7GQ7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0dc76d1 in 34 seconds
More details
- Looked at
35lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/app/traces/v4/query_builder_test.go:503
- Draft comment:
Test case names should be unique and descriptive to easily identify which test case failed. Consider renaming duplicate test cases like "Test buildTracesQuery" to something more specific. - Reason this comment was not posted:
Confidence changes required:50%
The test case names are not descriptive enough, especially when there are multiple tests with the same name. This can lead to confusion when tests fail, as it will be difficult to identify which specific test case failed.
2. pkg/query-service/app/traces/v4/query_builder_test.go:499
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_S7kxZS3jCVC5AN1h
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d399a9a in 1 minute and 44 seconds
More details
- Looked at
7733lines of code in74files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/rules/templates.go:265
- Draft comment:
The regex for handling{{$variable}}syntax should not skip special cases for{{$threshold}}and{{$value}}as they are already handled in the template functions. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. pkg/query-service/rules/templates.go:265
- Draft comment:
ThepreprocessTemplatefunction is incorrectly handling the{{$variable}}syntax by not skipping the special cases for{{$threshold}}and{{$value}}. This could lead to incorrect template processing. Ensure these special cases are skipped. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_8Q56L5Cmifq9Elw8
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
attribute_string_messageStaticFieldsTraceswill be more visible in the future PR's for enrichment.Part of #5713
Important
Introduces v4 query builder for traces with resource filters, common column format, and enhanced query construction and testing.
v4/query_builder.gofor handling trace queries with resource filters and a common format for materialized columns.resourcepackage for building resource subqueries inv4/query_builder.go.constants.gofor trace queries.getClickHouseTracesColumnType,getClickHouseTracesColumnDataType, andgetColumnNameinv4/query_builder.gofor column handling.buildTracesFilterQueryandhandleEmptyValuesInGroupByinv4/query_builder.gofor query construction.orderByandorderByAttributeKeyTagsinv4/query_builder.gofor ordering logic.query_builder_test.gofor v4 with tests for new functions likegetClickHouseTracesColumnType,getColumnName, andbuildTracesQuery.AggregateOperatorToPercentileandAggregateOperatorToSQLFuncinv3/query_builder.gofor reuse in v4.constants.gowith new constants for v3 and v4 trace handling.This description was created by
for d399a9a. It will automatically update as commits are pushed.