-
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
fix(Traces Explorer): prevent duplicate API calls to query_range in traces explorer #6677
base: main
Are you sure you want to change the base?
Conversation
…g of initQueryBuilderData
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 9e1aa02 in 1 minute and 22 seconds
More details
- Looked at
181
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/TracesExplorer/ListView/index.tsx:80
- Draft comment:
The useMemo hook has a redundant second array of dependencies. Remove the second array to avoid confusion and potential errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is incorrect. In useMemo, the first array is the value being memoized, and the second array is the dependencies that determine when to recompute that value. This is the standard and correct way to use useMemo. Removing the second array would break the memoization functionality.
Could there be a performance optimization opportunity by reordering the dependencies to match the order of usage? Could there be unnecessary dependencies?
While the order of dependencies could potentially be cleaned up, the current implementation is functionally correct and follows React's useMemo pattern. The dependencies listed are all necessary as they are used in the memoized value.
The comment should be deleted as it is incorrect - the second array is a required part of useMemo's API and is being used correctly here.
2. frontend/src/providers/QueryBuilder.tsx:836
- Draft comment:
The useEffect hook is missing 'currentQuery' as a dependency. Add it to ensure the effect runs correctly when 'currentQuery' changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The effect only uses location.pathname to track navigation changes and reset some state when navigation occurs. currentQuery is not referenced in the effect body at all, so adding it as a dependency would be incorrect and could cause unnecessary re-runs of the effect. The current dependency array correctly lists only the values used in the effect.
Could I be missing some subtle reason why currentQuery should be tracked? Perhaps there's some edge case where currentQuery changes would need to trigger this navigation reset logic?
No - this effect's sole purpose is to track navigation changes via pathname and reset state when navigation occurs. currentQuery changes should not trigger this navigation-specific logic. The current dependencies are correct.
The comment suggesting to add currentQuery as a dependency is incorrect and should be deleted. The current dependency array correctly includes only location.pathname which is the only value used in the effect.
Workflow ID: wflow_yRJjx5FIKCBR4uKM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
…ce-when-trace-explorer-page-is-loaded
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.
Remove the debuggers, rest looks good !
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 753824b in 25 seconds
More details
- Looked at
38
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/providers/QueryBuilder.tsx:99
- Draft comment:
Good use ofuseRef
to trackcurrentPathnameRef
and prevent unnecessary re-renders and duplicate API calls. - Reason this comment was not posted:
Confidence changes required:0%
The use ofuseRef
forcurrentPathnameRef
is appropriate for tracking the pathname without causing re-renders. The initialization withlocation.pathname
is crucial to prevent unnecessary re-renders and duplicate API calls, as described in the PR.
2. frontend/src/providers/QueryBuilder.tsx:95
- Draft comment:
Avoid using thecomponent/index.tsx
file 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_Fltwb9UNaRRWpkOr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
…ce-when-trace-explorer-page-is-loaded
e76b7e2
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 e76b7e2 in 38 seconds
More details
- Looked at
32
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/pages/TracesExplorer/__test__/TracesExplorer.test.tsx:112
- Draft comment:
Redundantjest.mock('react-redux', ...)
statement. It's already defined earlier in the file. Consider removing one to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment claims there's an earlier mock of react-redux, but I don't see one in the file. There are other mocks (react-router-dom, uplot, etc.) but no earlier mock of react-redux. The comment appears to be incorrect. Since the comment is wrong about a basic fact, it should be deleted.
Could there be another file that's importing and mocking react-redux? Could the mock be defined in a test setup file?
Even if react-redux is mocked elsewhere, this mock is extending the actual react-redux with a specific useSelector implementation, which would be needed for this test file. The comment is still incorrect about redundancy.
The comment should be deleted because it is factually incorrect - there is no earlier mock of react-redux in this file that would make this mock redundant.
2. frontend/src/pages/TracesExplorer/__test__/TracesExplorer.test.tsx:109
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_LaVvHiEReo4X5EOq
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
location.pathname
as the initial value for currentPathnameRef, absense of this causesstagedQuery && location.pathname !== currentPathnameRef.current
to be truthy (sincecurrentPathnameRef.current
is null), and therefore causes duplicate triggering ofinitQueryBuilderData(compositeQueryParam)
; in the useEffect of line ~784Related Issues / PR's
close #6308
Previous PR: #6640
Screenshots
Before:
2024-12-16.09-15-35.mov
After:
2024-12-16.09-19-47.mov
Affected Areas and Manually Tested Areas
Important
Prevent duplicate API calls in Traces Explorer by memoizing query keys and adding conditional checks in
index.tsx
,Filter.tsx
, andQueryBuilder.tsx
.queryKey
inindex.tsx
to prevent duplicate API calls in Traces Explorer.useGetQueryRange
inindex.tsx
is now conditional on!timeRangeUpdateLoading
.Filter.tsx
, added check to prevent redundant redirection ifcurrentQuery
matchespreparedQuery
.currentPathnameRef
inQueryBuilder.tsx
to prevent duplicateinitQueryBuilderData
calls.index.tsx
andQueryBuilder.tsx
for better readability and performance.This description was created by for e76b7e2. It will automatically update as commits are pushed.