-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: use search v2 component for traces #7537
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
base: main
Are you sure you want to change the base?
Conversation
…ta types for the same key
… non-logs data sources
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 2d24a9e in 1 minute and 17 seconds
More details
- Looked at
156
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
11
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/components/Query/Query.tsx:455
- Draft comment:
Refactoring the condition to use[DataSource.LOGS, DataSource.TRACES].includes(query.dataSource)
is concise; ensure downstream logic handles both data sources properly. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchDropdown.tsx:49
- Draft comment:
Using classnames with the modifier 'non-logs-data-source' is good; verify that the corresponding CSS class is defined in the stylesheet. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:1017
- Draft comment:
Rendering when the data source is TRACES is a neat extension; ensure the component styling uses design tokens instead of inline values. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. frontend/src/hooks/queryBuilder/useOptions.ts:169
- Draft comment:
Removing duplicate options by comparing only label and value seems appropriate; confirm that excluding dataType in the duplicate check is intentional. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:971
- Draft comment:
Avoid using inline styles. The inline style prop (selectStyle) is passed directly to the Select component. Consider moving these styles to a CSS class or using styled components to keep styling centralized. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:295
- Draft comment:
The string 'body' is hardcoded in multiple places (e.g. when setting a default attribute key). Consider extracting it into a constant to avoid duplication and potential typos in the future. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/hooks/queryBuilder/useOptions.ts:64
- Draft comment:
The parameters 'results' and 'result' are very similar in name, which can be confusing. Consider renaming them to more descriptive and distinct names to clarify their different roles. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. frontend/src/hooks/queryBuilder/useOptions.ts:141
- Draft comment:
The effect conditionally calls setOptions(newOptions) only when newOptions is non-empty. This might prevent the options state from being cleared when no options are available. Consider updating the state unconditionally with newOptions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. frontend/src/container/QueryBuilder/components/Query/Query.tsx:145
- Draft comment:
Typo: The function name 'handleToggleCollapsQuery' is likely a misspelling. It should probably be 'handleToggleCollapseQuery' to correctly reflect its purpose. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:389
- Draft comment:
Typo: In the comment on line 389, 'seperated' should be corrected to 'separated'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:827
- Draft comment:
Typo: In the comment on line 827, 'sycn' should be corrected to 'sync'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_NIrF5VUBkndIX5A6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
…component to search v2
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 7be90f4 in 1 minute and 8 seconds
More details
- Looked at
43
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:19
- Draft comment:
Snapshot tests are useful but can lead to brittle tests. Ensure these snapshots are maintained and updated intentionally when UI changes. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:8
- Draft comment:
Test descriptions and conditions are clear, ensuring that 'span-scope-selector' is rendered only for TRACES data source. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:19
- Draft comment:
Using a full snapshot test here (container.toMatchSnapshot) may be brittle as minor UI changes could require frequent snapshot updates. Consider adding (or replacing with) more targeted assertions if full snapshot validation isn’t critical. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The test already has a specific assertion checking for the presence of the component using getByTestId. The snapshot test provides additional coverage for the full rendered output. Having both types of tests is actually a good practice - snapshots for catching unexpected changes and specific assertions for core functionality. The comment seems to ignore that targeted assertions are already present.
The comment raises a valid general point about snapshot tests being brittle. Maybe there are more specific assertions we could add beyond just component presence.
While snapshot brittleness is a real concern, this test already follows best practices by combining snapshot and targeted assertions. The comment doesn't provide specific suggestions for additional assertions.
Delete the comment as it doesn't acknowledge the existing targeted assertions and doesn't provide specific actionable improvements.
Workflow ID: wflow_0frv1iDMQ6f4F0ZK
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.
Functionality LGTM
frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx
Show resolved
Hide resolved
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.
Minor query. Rest everything looks fine.
Approving for now
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 a79c8c8 in 2 minutes and 6 seconds
More details
- Looked at
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:702
- Draft comment:
UsingtagKey.trim().length > 0
instead of!isEmpty(tagKey)
is clearer for checking non-empty strings. The change from usingisEqual(val.key, tagKey)
toval.key === tagKey
is appropriate here sincetagKey
is a primitive string. Ensure thattagKey
is always a string to avoid unexpected type coercions. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:702
- Draft comment:
Replacing lodash's isEmpty(tagKey) with tagKey.trim().length > 0 ensures that strings containing only whitespace are correctly treated as empty. Also, using strict equality (===) instead of isEqual for comparing tag keys is simpler and more performant when the keys are expected to be strings. Please confirm that tagKey is always a string and that this behavior matches the intended handling of whitespace. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment makes valid technical points about the changes: - Using trim().length > 0 is more correct for handling whitespace-only strings than isEmpty()
- Using === is more performant than isEqual() for simple string comparison
However, the comment ends by asking the author to confirm assumptions, which violates our rules about not asking for confirmations.
The technical feedback is good, but the comment's request for confirmation makes it less actionable. Also, since the author made these improvements themselves, maybe they already know this.
While the technical points are valid, the author has already implemented these improvements, so the comment is more explanatory than actionable. The request for confirmation should be removed.
The comment should be deleted because it is primarily explaining changes the author already made and includes a non-actionable request for confirmation.
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:392
- Draft comment:
Typo: 'seperated' should be 'separated' in the comment describing comma separated values. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:835
- Draft comment:
Typo: 'sycn' should be corrected to 'sync' in the comment ensuring that tags stay in sync with the current query. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_XSlbo9FPt0FUEjLl
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.
❌ Changes requested. Incremental review on 698b8d6 in 1 minute and 44 seconds
More details
- Looked at
193
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:123
- Draft comment:
Using 'await' with a synchronous function. 'selectOption' does not return a Promise, so 'await' is redundant. Consider removing 'await' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment is technically correct - the await is unnecessary since selectOption is synchronous. However, this is a test file and the await doesn't cause any actual issues. The test still works correctly either way. This feels like a very minor nitpick that doesn't meaningfully improve code quality.
The await could be intentional to make the test more readable and consistent with other async tests in the file. Line 157 uses await with screen.findByText() which is async.
While consistency is important, using await with sync functions can be misleading and confusing to readers. However, this is such a minor issue that commenting on it creates more noise than value.
Delete the comment. While technically correct, this is too minor of an issue to warrant a PR comment, especially in test code.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:125
- Draft comment:
The test 'should add isRoot filter when selecting ROOT_SPANS' uses 'await selectOption(...)', but selectOption is defined synchronously. Consider removing the async/await if no asynchronous actions are needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The selectOption function itself is synchronous, but it's triggering UI updates that might need to settle. React Testing Library's fireEvent can sometimes need time for state updates to propagate. However, if we're just checking mockRedirectWithQueryBuilderData.toHaveBeenCalled(), that should be synchronous since it's just a mock function.
I might be missing some async behavior in the underlying SpanScopeSelector component that necessitates the await. The component code isn't visible in the diff.
Without seeing the SpanScopeSelector component code, I can't be 100% certain that the async/await is unnecessary. The author may have had a reason for making it async.
Since I don't have strong evidence that the async/await is definitely wrong, and removing it could potentially break the test, I should not keep this comment.
Workflow ID: wflow_joKTSUpgo6TXTExc
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.
.../src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx
Outdated
Show resolved
Hide resolved
.../src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx
Outdated
Show resolved
Hide resolved
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.
❌ Changes requested. Incremental review on 253c1cf in 3 minutes and 39 seconds
More details
- Looked at
67
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:281
- Draft comment:
Duplicate dependency in useMemo: 'attributeValues?.payload?.stringAttributeValues?.length' is listed twice. Consider removing the duplicate or verifying if a different property was intended. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:280
- Draft comment:
Duplicate dependency detected: 'attributeValues?.payload?.stringAttributeValues?.length' appears twice in the useMemo dependencies for 'attributeValuesPayload'. Please remove the duplicate to ensure accurate dependency tracking. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:713
- Draft comment:
Good refactoring: using the memoized 'attributeKeysPayload' instead of direct access improves consistency. However, since the dependency array only tracks the payload’s length, ensure that changes to the payload contents (without length change) are not missed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a valid concern - if the contents of attributeKeys change without the length changing, the memoized value won't update. However, this seems unlikely in practice since attributeKeys are typically added/removed rather than modified in place. The comment is speculative ("ensure that...") and asks for verification rather than pointing out a clear issue.
The comment identifies a real edge case in the memoization logic. Changes to key properties without length changes could cause stale data.
While technically correct, the comment violates the rules by being speculative and asking for verification. It doesn't point to a clear issue that needs fixing.
The comment should be deleted because it is speculative and asks for verification rather than identifying a clear issue that needs to be fixed.
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:789
- Draft comment:
Consistent use of the memoized 'attributeValuesPayload' is a clear improvement. Verify that using only the lengths of the nested arrays as dependencies is sufficient, as content updates that don’t change the array lengths might not be captured. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify their implementation, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue with the code.
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:403
- Draft comment:
Typo: 'seperated' should be corrected to 'separated' in the comment describing comma separated values. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx:847
- Draft comment:
Typo: 'sycn' should be corrected to 'sync' in the comment that talks about keeping tags in sync with the query. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_xBlh39zSOjKNzXlr
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.
frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/QueryBuilderSearchV2.tsx
Outdated
Show resolved
Hide resolved
253c1cf
to
0e2bca1
Compare
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.
Important
Looks good to me! 👍
Reviewed 0e2bca1 in 47 seconds. Click for details.
- Reviewed
86
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/QueryBuilderSearchV2.test.tsx:80
- Draft comment:
Avoid using a generic 'as any' here. Providing a more specific type for the QueryBuilderContext value will improve type safety in tests. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/jest.config.ts:17
- Draft comment:
Ensure the addition of 'isolatedModules: true' is intended. This flag can speed up compilation by skipping type-checks, so verify its effect on test accuracy. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/QueryBuilderSearchV2.test.tsx:92
- Draft comment:
Good refactor: extracting duplicate aggregate keys data into a constant improves maintainability and reduces repetition. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/QueryBuilderSearchV2.test.tsx:115
- Draft comment:
Similarly, extracting the aggregate values mock data into a constant aids in clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/QueryBuilderSearchV2.test.tsx:128
- Draft comment:
Consider removing the commented-out mock for 'useUrlQuery' if it's no longer in use to keep the test file clean. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_9rzpEGY0JfRIUGyE
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
0e2bca1
to
ac88da8
Compare
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.
Important
Looks good to me! 👍
Reviewed ac88da8 in 36 seconds. Click for details.
- Reviewed
33
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:40
- Draft comment:
Good improvement: using TagFilterItem instead of any[] for createQueryWithFilters enhances type safety. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:63
- Draft comment:
Consider replacing the use of 'as any' for the context value with a more specific type to improve type clarity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:40
- Draft comment:
Good change – typing thefilters
parameter as TagFilterItem[] instead of any[] improves type safety. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:92
- Draft comment:
Nice update typingupdatedQuery
as Query in assertFilterAdded instead of any, which clarifies expected structure. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:127
- Draft comment:
The test uses 'await selectOption(...)' while selectOption is defined synchronously. Consider removing the unnecessary await for clarity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. frontend/src/container/QueryBuilder/filters/QueryBuilderSearchV2/__test__/SpanScopeSelector.test.tsx:66
- Draft comment:
Consider properly typing the QueryBuilderContext value instead of using 'as any' to avoid ESLint disables and improve type safety in tests. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_SdcH9VRGHDI3Er5v
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Functionality LGTM
Summary
Related Issues / PR's
close #7160
close #7355
Screenshots
Before:
2025-04-06.12-31-14.mov
After:
2025-04-06.12-32-31.mov
Affected Areas and Manually Tested Areas
Traces explorer
✔ Suggestions
✔ Search
✔ Multiple types
✔ stage and run sends the correct filters
✔ Display the span scope dropdown
Dashboard (create / edit panel)
✔ Suggestions
✔ Search
✔ Multiple types
✔ stage and run sends the correct filters
✔ Display the span scope dropdown
Alert
✔ Suggestions
✔ Search
✔ Multiple types
✔ stage and run sends the correct filters
✔ Display the span scope dropdown
Important
Introduce
QueryBuilderSearchV2
for trace data sources with span filtering and multiple data type support, replacingQueryBuilderSearch
in specific contexts.QueryBuilderSearch
withQueryBuilderSearchV2
forDataSource.TRACES
inQuery.tsx
.DataSource.TRACES
inQueryBuilderSearchV2.tsx
.QueryBuilderSearchV2.tsx
.QueryBuilderSearchV2.test.tsx
.QueryBuilderSearchV2.test.tsx
.SpanScopeSelector
inSpanScopeSelector.test.tsx
.QueryBuilderSearchV2.styles.scss
for new dropdown and span scope selector.SpanScopeSelector
toQueryBuilderSearchV2
directory and update imports.This description was created by
for ac88da8. You can customize this summary. It will automatically update as commits are pushed.