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.

Sign up for GitHub

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

Jump to bottom

[lens] changing disabled filter causes lens panel fetch when embedded on dashboard #151223

Closed
nreese opened this issue Feb 14, 2023 · 5 comments · Fixed by #151799
Closed

[lens] changing disabled filter causes lens panel fetch when embedded on dashboard #151223

nreese opened this issue Feb 14, 2023 · 5 comments · Fixed by #151799
Assignees
@nreese
Labels
bug Fixes for quality problems that affect the customer experience Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. regression Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@nreese
Copy link
Contributor

nreese commented Feb 14, 2023

Originally fixed by #41144

Steps to view problem

  1. install web logs sample data set
  2. create dashboard with lens panel
  3. add filter pill and disable it
  4. open browser new work tab.
  5. edit filter by toggling exclude
  6. notice how each action results in bsearch network request, even though the results will be the same

bug occurs because diff check uses fastIsEqual to check filters instead of using onlyDisabledFiltersChanged

https://github.com/elastic/kibana/blob/main/x-pack/plugins/lens/public/embeddable/embeddable.tsx#L434


    // Update search context and reload on changes related to search
    this.inputReloadSubscriptions.push(
      this.getUpdated$()
        .pipe(map(() => this.getInput()))
        .pipe(
          distinctUntilChanged((a, b) =>
            fastIsEqual(
              [a.filters, a.query, a.timeRange, a.searchSessionId],
              [b.filters, b.query, b.timeRange, b.searchSessionId]
            )
          ),
          skip(1)
        )
        .subscribe(async (input) => {
          this.onContainerStateChanged(input);
        })
    );
  }
@nreese nreese added bug Fixes for quality problems that affect the customer experience regression Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Feb 14, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

@nreese
Copy link
Contributor Author

nreese commented Feb 14, 2023 •
edited
Loading

Blocked by #151224.

Dashboard updates searchSessionId updated when disabled filter changes so even if this issue is fixed for lens embeddable, the searchSessionId check would still cause a new search

@nreese nreese added the blocked label Feb 14, 2023
@nreese nreese mentioned this issue Feb 14, 2023
Closed
@stratoula stratoula added the impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. label Feb 15, 2023
@nreese nreese removed the blocked label Feb 21, 2023
@nreese
Copy link
Contributor Author

nreese commented Feb 21, 2023

#151224 is resolved and this issue is unblocked

@drewdaemon drewdaemon self-assigned this Feb 21, 2023
@drewdaemon
Copy link
Contributor

drewdaemon commented Feb 21, 2023 •
edited
Loading

I can't reproduce this issue as it is stated. True, the filter comparison check in the embeddable is faulty, but no network request is created. I think it's probably because of the expression request caching layer.

@drewdaemon drewdaemon assigned nreese and unassigned drewdaemon Feb 21, 2023
@drewdaemon
Copy link
Contributor

Assigned to @nreese since his current work on unifying this "listener" logic across all our embeddables will resolve everything. Thank you!

@nreese nreese mentioned this issue Feb 21, 2023
Merged
nreese added a commit that referenced this issue Mar 1, 2023
@nreese @kibanamachine
[embeddable] centralize should fetch embeddable observable logic (#15…
afb251c 
…1799)

Fixes #151223 and
#151128

PR does the following
1) creates `shouldFetch$` method that centralizes logic for checking
when an embeddable should fetch.
2) updates Lens and Maps embeddable to use `shouldFetch$`
3) Adds unit tests for Maps embeddable to capture behavior of unique
edge cases

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
dej611 pushed a commit to dej611/kibana that referenced this issue Mar 7, 2023
@nreese
[embeddable] centralize should fetch embeddable observable logic (ela…
6c75706 
…stic#151799)

Fixes elastic#151223 and
elastic#151128

PR does the following
1) creates `shouldFetch$` method that centralizes logic for checking
when an embeddable should fetch.
2) updates Lens and Maps embeddable to use `shouldFetch$`
3) Adds unit tests for Maps embeddable to capture behavior of unique
edge cases

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit afb251c)
dej611 added a commit that referenced this issue Mar 7, 2023
@dej611 @nreese
[8.7] [embeddable] centralize should fetch embeddable observable logic (
09ce818 
#151799) (#152838)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[embeddable] centralize should fetch embeddable observable logic
(#151799)](#151799)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"reese.nathan@elastic.co"},"sourceCommit":{"committedDate":"2023-03-01T01:47:48Z","message":"[embeddable]
centralize should fetch embeddable observable logic (#151799)\n\nFixes
#151223
and\r\nhttps://github.com//issues/151128\r\n\r\nPR does
the following\r\n1) creates `shouldFetch# Backport

This will backport the following commits from `main` to `8.7`:
- [[embeddable] centralize should fetch embeddable observable logic
(#151799)](#151799)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT method that centralizes logic for checking\r\nwhen an
embeddable should fetch.\r\n2) updates Lens and Maps embeddable to use
`shouldFetch# Backport

This will backport the following commits from `main` to `8.7`:
- [[embeddable] centralize should fetch embeddable observable logic
(#151799)](#151799)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT \r\n3) Adds unit tests for Maps embeddable to capture
behavior of unique\r\nedge cases\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"afb251c624552024076ae75890de9c7d5f4458f0","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Visualizations","Feature:Embedding","Team:Presentation","release_note:skip","backport:skip","Feature:Maps","v8.8.0"],"number":151799,"url":"https://github.com/elastic/kibana/pull/151799","mergeCommit":{"message":"[embeddable]
centralize should fetch embeddable observable logic (#151799)\n\nFixes
#151223
and\r\nhttps://github.com//issues/151128\r\n\r\nPR does
the following\r\n1) creates `shouldFetch# Backport

This will backport the following commits from `main` to `8.7`:
- [[embeddable] centralize should fetch embeddable observable logic
(#151799)](#151799)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT method that centralizes logic for checking\r\nwhen an
embeddable should fetch.\r\n2) updates Lens and Maps embeddable to use
`shouldFetch# Backport

This will backport the following commits from `main` to `8.7`:
- [[embeddable] centralize should fetch embeddable observable logic
(#151799)](#151799)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT \r\n3) Adds unit tests for Maps embeddable to capture
behavior of unique\r\nedge cases\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"afb251c624552024076ae75890de9c7d5f4458f0"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/151799","number":151799,"mergeCommit":{"message":"[embeddable]
centralize should fetch embeddable observable logic (#151799)\n\nFixes
#151223
and\r\nhttps://github.com//issues/151128\r\n\r\nPR does
the following\r\n1) creates `shouldFetch# Backport

This will backport the following commits from `main` to `8.7`:
- [[embeddable] centralize should fetch embeddable observable logic
(#151799)](#151799)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT method that centralizes logic for checking\r\nwhen an
embeddable should fetch.\r\n2) updates Lens and Maps embeddable to use
`shouldFetch# Backport

This will backport the following commits from `main` to `8.7`:
- [[embeddable] centralize should fetch embeddable observable logic
(#151799)](#151799)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT \r\n3) Adds unit tests for Maps embeddable to capture
behavior of unique\r\nedge cases\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"afb251c624552024076ae75890de9c7d5f4458f0"}}]}]
BACKPORT-->

Co-authored-by: Nathan Reese <reese.nathan@elastic.co>
sloanelybutsurely pushed a commit to sloanelybutsurely/kibana that referenced this issue Mar 8, 2023
@nreese @kibanamachine
[embeddable] centralize should fetch embeddable observable logic (ela…
7265396 
…stic#151799)

Fixes elastic#151223 and
elastic#151128

PR does the following
1) creates `shouldFetch$` method that centralizes logic for checking
when an embeddable should fetch.
2) updates Lens and Maps embeddable to use `shouldFetch$`
3) Adds unit tests for Maps embeddable to capture behavior of unique
edge cases

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this issue Mar 10, 2023
@nreese @kibanamachine @bmorelli25
[embeddable] centralize should fetch embeddable observable logic (ela…
fa7c682 
…stic#151799)

Fixes elastic#151223 and
elastic#151128

PR does the following
1) creates `shouldFetch$` method that centralizes logic for checking
when an embeddable should fetch.
2) updates Lens and Maps embeddable to use `shouldFetch$`
3) Adds unit tests for Maps embeddable to capture behavior of unique
edge cases

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
nkhristinin pushed a commit that referenced this issue Mar 22, 2023
@nreese @kibanamachine @nkhristinin
[embeddable] centralize should fetch embeddable observable logic (#15…
af53dd0 
…1799)

Fixes #151223 and
#151128

PR does the following
1) creates `shouldFetch$` method that centralizes logic for checking
when an embeddable should fetch.
2) updates Lens and Maps embeddable to use `shouldFetch$`
3) Adds unit tests for Maps embeddable to capture behavior of unique
edge cases

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nreese nreese

Labels
bug Fixes for quality problems that affect the customer experience Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. regression Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[embeddable] centralize should fetch embeddable observable logic nreese/kibana
4 participants
@drewdaemon @nreese @elasticmachine @stratoula