-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: enable the new where clause for all the log based queries #6671
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 de9a575 in 21 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/components/Query/Query.tsx:460
- Draft comment:
The conditionisLogsExplorerPage || query.dataSource === DataSource.LOGS
is repeated multiple times. Consider extracting it into a variable for better readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The change in the condition is straightforward and aligns with the PR description. However, the condition can be simplified for better readability.
Workflow ID: wflow_hKyCZaSuOJQmlJ1B
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this 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 77dffb9 in 27 seconds
More details
- Looked at
93
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:148
- Draft comment:
The useMemo hook forisLogsDataSource
is unnecessary since it only checks a boolean condition. Consider using a simple variable assignment instead. This pattern is also present in other parts of the code, such asisMetricsDataSource
. - Reason this comment was not posted:
Confidence changes required:50%
The useMemo hook is used to determine if the data source is logs, but the dependency array is not necessary since the value is derived directly from the query object. This pattern is repeated in multiple places.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:145
- Draft comment:
The useMemo hook forisLogsDataSource
is unnecessary since it only checks a boolean condition. Consider using a simple variable assignment instead. This pattern is also present in other parts of the code, such asisMetricsDataSource
. - Reason this comment was not posted:
Confidence changes required:50%
The useMemo hook is used to determine if the data source is metrics, but the dependency array is not necessary since the value is derived directly from the query object. This pattern is repeated in multiple places.
3. frontend/src/container/QueryBuilder/components/Query/Query.tsx:453
- Draft comment:
Avoid using inline styles. Replace with CSS classes or styled components. This applies to other instances in this file as well. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:648
- Draft comment:
Avoid using inline styles. Replace with CSS classes or styled components. This applies to other instances in this file as well. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_1PHWTcEumLI1YnJQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Related Issues / PR's
contributes to #6317
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Enables
QueryBuilderSearchV2
for all log-based queries by checkingquery.dataSource
inQuery.tsx
andQueryBuilderSearchV2.tsx
.QueryBuilderSearchV2
for all log-based queries inQuery.tsx
by checkingquery.dataSource === DataSource.LOGS
.isLogsExplorerPage
withisLogsDataSource
inQueryBuilderSearchV2.tsx
to determine if the data source is logs.This description was created by for 77dffb9. It will automatically update as commits are pushed.