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 x-ray dashboards run some queries twice on load #40844

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

kulyk
Copy link
Member

@kulyk kulyk commented Apr 1, 2024

Closes #36775

X-ray code has a check that should rerun queries when parameter values changes. Apparently, the first time it's triggered, it receives two maps: one is just empty ({}), and another has all the parameter values set to null. Even though they both basically mean "dashboard filters don't have values set", underscore's isEqual treats them as different, and that triggers extra query reruns.

This PR is rather a band-aid, we should invest some time into unifying this logic with regular and public/embedded dashboards that don't have this issue in the first place

How to verify

  1. Open "Browse data", pick any table and x-ray it (click the bolt icon)
  2. Ensure there're no canceled /api/dataset requests in the Network tab

@kulyk kulyk added backport Automatically create PR on current release branch on merge .Team/DashViz Dashboard and Viz team labels Apr 1, 2024
@kulyk kulyk requested review from a team April 1, 2024 14:41
@kulyk kulyk self-assigned this Apr 1, 2024
Copy link

replay-io bot commented Apr 1, 2024

Status Complete ↗︎
Commit aebb2e9
Results
⚠️ 5 Flaky
2386 Passed

Copy link
Member

@alxnddr alxnddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having a spec for this can be handy to avoid reintroducing the issue. Also, since it is a quick fix, can you please create a tech debt issue to remove the unnecessary initial render with an empty object?

@kulyk
Copy link
Member Author

kulyk commented Apr 1, 2024

I think having a spec for this can be handy to avoid reintroducing the issue

I'm afraid an E2E test here would be very flaky and unreliable. The number of redundant requests is different every time, even on a local machine with no network latency

Also, since it is a quick fix, can you please create a tech debt issue to remove the unnecessary initial render with an empty object?

I think ideally we should migrate x-rays to the same dashboard fetching logic as other kinds of dashboard (mainly via Dashboard.tsx component for now). WDYT?

@alxnddr
Copy link
Member

alxnddr commented Apr 1, 2024

I think ideally we should migrate x-rays to the same dashboard fetching logic as other kinds of dashboard (mainly via Dashboard.tsx component for now). WDYT?

Agree, that would be ideal

@kulyk kulyk merged commit 45b2f14 into master Apr 1, 2024
107 checks passed
@kulyk kulyk deleted the 36775-fix-redundant-xray-queries branch April 1, 2024 16:07
@kulyk
Copy link
Member Author

kulyk commented Apr 1, 2024

@metabase-bot backport release-x.48.x

github-actions bot pushed a commit that referenced this pull request Apr 1, 2024
* Fix parameter values comparison in x-rays

* Skip rerun when `parameterValues` are not initialized

* Remove not used code
github-actions bot pushed a commit that referenced this pull request Apr 1, 2024
* Fix parameter values comparison in x-rays

* Skip rerun when `parameterValues` are not initialized

* Remove not used code
metabase-bot bot added a commit that referenced this pull request Apr 1, 2024
* Fix parameter values comparison in x-rays

* Skip rerun when `parameterValues` are not initialized

* Remove not used code

Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>
metabase-bot bot added a commit that referenced this pull request Apr 1, 2024
* Fix parameter values comparison in x-rays

* Skip rerun when `parameterValues` are not initialized

* Remove not used code

Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>
noahmoss pushed a commit that referenced this pull request Apr 3, 2024
* Fix parameter values comparison in x-rays

* Skip rerun when `parameterValues` are not initialized

* Remove not used code
rafpaf pushed a commit that referenced this pull request Apr 3, 2024
* Fix parameter values comparison in x-rays

* Skip rerun when `parameterValues` are not initialized

* Remove not used code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/DashViz Dashboard and Viz team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We're sending twice the necessary number of requests on x-ray first load
2 participants