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(Traces Explorer): prevent duplicate API calls to query_range in traces explorer #6640

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

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Dec 15, 2024

Summary

  • Memoize the query key to ensure that we don't have multiple query keys.
  • Make API call only when the time range state is not loading (while the time range state is being updated).
  • Added a check in the Filter component to avoid redundant redirection when the current query matches the prepared query.

Related Issues / PR's

close #6308

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 optimizing conditions for API execution and redirection.

  • Behavior:
    • Memoize queryKey in ListView to prevent duplicate API calls.
    • API call in ListView only when timeRangeUpdateLoading is false.
    • In Filter, prevent redirection if currentQuery equals preparedQuery.
  • Code Improvements:
    • Initialize currentPathnameRef in QueryBuilderProvider to prevent duplicate query initialization.

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

@github-actions github-actions bot added the bug Something isn't working label Dec 15, 2024
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 ed52d4f in 42 seconds

More details
  • Looked at 124 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/container/TracesExplorer/ListView/index.tsx:80
  • Draft comment:
    The dependencies array for useMemo on line 69 should include 'options' to ensure that changes to 'options?.selectColumns' are tracked.
		[stagedQuery, panelType, globalSelectedTime, paginationConfig, options, maxTime, minTime]
  • 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 change in the dependencies array of a useMemo hook, which is part of the diff. The suggestion to include 'options' instead of 'options?.selectColumns' could be valid if 'options' itself changes and affects the memoized value. However, the current code already tracks 'options?.selectColumns', which might be sufficient if only the 'selectColumns' property is relevant.
    I might be missing the context of how 'options' is used elsewhere in the component. If 'options' changes in a way that affects the memoized value, the comment could be valid.
    The current code specifically tracks 'options?.selectColumns', which suggests that only changes to 'selectColumns' are relevant. Without evidence that 'options' changes in a way that affects the memoized value, the comment might not be necessary.
    The comment should be deleted unless there is strong evidence that 'options' changes in a way that affects the memoized value. The current tracking of 'options?.selectColumns' seems sufficient.
2. frontend/src/container/TracesExplorer/ListView/index.tsx:111
  • Draft comment:
    Consider adding a check for 'stagedQuery' being defined to prevent unnecessary API calls when 'stagedQuery' is undefined or null.
				!!stagedQuery &&
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/pages/TracesExplorer/Filter/Filter.tsx:207
  • Draft comment:
    Consider using a deep comparison for selectedFilters in the useEffect dependency array to prevent unnecessary executions of handleRun when selectedFilters are deeply equal but not referentially equal.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_tNscXNbiL4OTiNDu


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

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 9e1aa02 in 28 seconds

More details
  • Looked at 56 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/providers/QueryBuilder.tsx:809
  • Draft comment:
    The dependency array for this useEffect should not include 'stagedQuery' as it is not used within the effect. Consider removing it to avoid unnecessary re-renders.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/providers/QueryBuilder.tsx:828
  • Draft comment:
    Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This comment applies to the entire file.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses inline comments for debugging purposes, which is not a violation of the rules provided. However, the use of inline styles or hardcoded color values is not present in this file, so no comments are needed for those rules.

Workflow ID: wflow_E6XmtoBEauy1VsoQ


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

@vikrantgupta25 vikrantgupta25 self-assigned this Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FE[TRACES]: Query range api is triggered twice when trace explorer page is loaded.
2 participants