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

[Unified histogram][ES|QL] Extra fetches in specific scenarios #165192

Open
stratoula opened this issue Aug 30, 2023 · 7 comments
Open

[Unified histogram][ES|QL] Extra fetches in specific scenarios #165192

stratoula opened this issue Aug 30, 2023 · 7 comments
Labels
bug Fixes for quality problems that affect the customer experience Feature:Discover Discover Application Feature:ES|QL ES|QL related features in Kibana Feature:UnifiedHistogram Issues related to the Unified Histogram plugin impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.

Comments

@stratoula
Copy link
Contributor

stratoula commented Aug 30, 2023 •
edited
Loading

Describe the feature:
For some reason when we enabled EQSL in Discover the esql request is sent 4 times (instead of 2) when opening a saved search #146971 (comment)

In all other cases it works as expected. I tried to understand what is going on but I can't, possibly because I am not very comfortable with the unified histogram case.

It will be cool to investigate and understand what is going on here.

Other cases with extra fetches are:

  • 1 extra fetch on chart hide
  • 1 extra fetch on page reload when a chart is visible
  • 1 extra fetch when changing from an index where the vis is hidden to one where it's shown
  • 3 extra fetches when navigating back from Dashboard to Discover after adding an ES|QL Discover vis
  • 3 extra fetches when saving or opening a saved search
@stratoula stratoula added bug Fixes for quality problems that affect the customer experience Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Feature:UnifiedHistogram Issues related to the Unified Histogram plugin Feature:ES|QL ES|QL related features in Kibana labels Aug 30, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@davismcphee davismcphee mentioned this issue Sep 1, 2023
Merged
3 tasks
@stratoula stratoula changed the title [Unified histogram][ES|QL] Opening a saved search results in 4 requests [Unified histogram][ES|QL] Extra fetches is specific scenarios Sep 1, 2023
@davismcphee davismcphee added impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:needs-research This issue requires some research before it can be worked on or estimated labels Sep 8, 2023
@kertal kertal added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Sep 14, 2023
@stratoula stratoula changed the title [Unified histogram][ES|QL] Extra fetches is specific scenarios [Unified histogram][ES|QL] Extra fetches in specific scenarios Oct 16, 2023
@jughosta
Copy link
Contributor

jughosta commented Dec 7, 2023 •
edited
Loading

Hi @stratoula,

Can you please check if there are less requests with #171638 ?
I think it's because of the logic in src/plugins/unified_histogram/public/chart/hooks/use_total_hits.ts which I am changing in that PR but would like to verify.

If it works, I can create another PR for backporting.

@jughosta jughosta mentioned this issue Dec 7, 2023
Closed
10 tasks
@jughosta
Copy link
Contributor

jughosta commented Dec 7, 2023

Okay, created a smaller PR #172880 just for this issue.

@jughosta jughosta mentioned this issue Dec 8, 2023
Merged
4 tasks
@stratoula
Copy link
Contributor Author

@jughosta I totally missed the mention 🙈 I am reviewing the first PR now!

@stratoula
Copy link
Contributor Author

I can't reproduce the first 2 with this PR 🙌

jughosta added a commit that referenced this issue Jan 12, 2024
@jughosta @kibanamachine
@stratoula @davismcphee
[Discover] Move total hits counter from histogram to grid area. New c…
aa33843 
…ontrols in histogram. (#171638)

- Closes #168825
- Closes #171610
- Closes #167427
- Partially addresses #165192

## Summary

This PR moves the total hits counter closer to the grid, updates
histogram controls and introduces new panel toggle buttons for toggling
fields sidebar and histogram.

<img width="500" alt="Screenshot 2023-12-05 at 15 37 20"
src="https://github.com/elastic/kibana/assets/1415710/5b9bd771-1052-4205-849f-18c21cc299b8">
<img width="500" alt="Screenshot 2023-12-05 at 15 37 29"
src="https://github.com/elastic/kibana/assets/1415710/e5941b27-c497-4d7e-b461-68b66931475a">
<img width="500" alt="Screenshot 2023-12-05 at 15 37 37"
src="https://github.com/elastic/kibana/assets/1415710/97abd32e-9ff2-4d9a-b7e7-b9d6d9cf64db">
<img width="500" alt="Screenshot 2023-12-05 at 15 37 50"
src="https://github.com/elastic/kibana/assets/1415710/10f2b4f4-ec37-41c3-b78b-78c64e14d655">
<img width="400" alt="Screenshot 2023-12-05 at 15 37 59"
src="https://github.com/elastic/kibana/assets/1415710/ef2e28b2-f6ba-4ccb-aea4-3946ba2d5839">
<img width="300" alt="Screenshot 2023-12-05 at 15 38 05"
src="https://github.com/elastic/kibana/assets/1415710/07901ede-0bcb-46a6-a398-4562189fd54f">
<img width="500" alt="Screenshot 2023-12-05 at 15 40 38"
src="https://github.com/elastic/kibana/assets/1415710/17830115-2111-4b8f-ae40-7b5875c06879">
<img width="500" alt="Screenshot 2023-12-05 at 15 40 56"
src="https://github.com/elastic/kibana/assets/1415710/975d475b-280b-495a-b7b7-31c7ade5f21e">
<img width="500" alt="Screenshot 2023-12-05 at 15 43 08"
src="https://github.com/elastic/kibana/assets/1415710/38b6053a-e260-48d8-9591-3f3409df2876">

## Testing

When testing, please check collapsing/expanding the fields sidebar and
histogram. Also for ES|QL mode with suggestions, legacy table, no
results and error prompt, Field Statistics tab, data views without a
time field, light/dark themes.

### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Co-authored-by: Davis McPhee <[email protected]>
semd pushed a commit to semd/kibana that referenced this issue Jan 12, 2024
@jughosta @kibanamachine
@stratoula @davismcphee @semd
[Discover] Move total hits counter from histogram to grid area. New c…
62edc8b 
…ontrols in histogram. (elastic#171638)

- Closes elastic#168825
- Closes elastic#171610
- Closes elastic#167427
- Partially addresses elastic#165192

## Summary

This PR moves the total hits counter closer to the grid, updates
histogram controls and introduces new panel toggle buttons for toggling
fields sidebar and histogram.

<img width="500" alt="Screenshot 2023-12-05 at 15 37 20"
src="https://github.com/elastic/kibana/assets/1415710/5b9bd771-1052-4205-849f-18c21cc299b8">
<img width="500" alt="Screenshot 2023-12-05 at 15 37 29"
src="https://github.com/elastic/kibana/assets/1415710/e5941b27-c497-4d7e-b461-68b66931475a">
<img width="500" alt="Screenshot 2023-12-05 at 15 37 37"
src="https://github.com/elastic/kibana/assets/1415710/97abd32e-9ff2-4d9a-b7e7-b9d6d9cf64db">
<img width="500" alt="Screenshot 2023-12-05 at 15 37 50"
src="https://github.com/elastic/kibana/assets/1415710/10f2b4f4-ec37-41c3-b78b-78c64e14d655">
<img width="400" alt="Screenshot 2023-12-05 at 15 37 59"
src="https://github.com/elastic/kibana/assets/1415710/ef2e28b2-f6ba-4ccb-aea4-3946ba2d5839">
<img width="300" alt="Screenshot 2023-12-05 at 15 38 05"
src="https://github.com/elastic/kibana/assets/1415710/07901ede-0bcb-46a6-a398-4562189fd54f">
<img width="500" alt="Screenshot 2023-12-05 at 15 40 38"
src="https://github.com/elastic/kibana/assets/1415710/17830115-2111-4b8f-ae40-7b5875c06879">
<img width="500" alt="Screenshot 2023-12-05 at 15 40 56"
src="https://github.com/elastic/kibana/assets/1415710/975d475b-280b-495a-b7b7-31c7ade5f21e">
<img width="500" alt="Screenshot 2023-12-05 at 15 43 08"
src="https://github.com/elastic/kibana/assets/1415710/38b6053a-e260-48d8-9591-3f3409df2876">

## Testing

When testing, please check collapsing/expanding the fields sidebar and
histogram. Also for ES|QL mode with suggestions, legacy table, no
results and error prompt, Field Statistics tab, data views without a
time field, light/dark themes.

### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Co-authored-by: Davis McPhee <[email protected]>
@jughosta jughosta mentioned this issue Feb 19, 2024
Open
@jughosta jughosta mentioned this issue Apr 18, 2024
Draft
2 tasks
@jughosta
Copy link
Contributor

An update here:

Apparently request count e2e tests for ES|QL were deleted at some point and we are not testing it right now. I tried to restore the tests but they are quite flaky #181184

The main problem is that now the text based editor also triggers esql requests (for fields autocomplete) and this operation is unpredictable (sometimes several requests, sometimes none). I could not find a solution yet to isolate discover requests (chart + table) in e2e tests from the editor requests so it could be actually tested in a more controlled way.

Besides that, here are various types of fetches the Discover page makes:
Screenshot 2024-04-24 at 16 47 57
Screenshot 2024-04-24 at 16 28 58

The embeddable requests are particularly interesting because they are triggered by different app logic so there could be some concurrent triggers:
Screenshot 2024-04-24 at 16 47 32
Screenshot 2024-04-24 at 16 27 31
Screenshot 2024-04-24 at 16 25 38

Once I even caught that for ES|QL query with stats, the embeddable made a separate request although we passed the prefetched data from Discover via table prop to actually avoid extra requests:
Screenshot 2024-04-11 at 11 31 48
Screenshot 2024-04-11 at 11 32 59

@kertal kertal mentioned this issue Oct 15, 2024
Merged
2 tasks
@lukasolson lukasolson mentioned this issue Oct 30, 2024
Closed
@jughosta jughosta mentioned this issue Nov 21, 2024
Open
@kertal
Copy link
Member

kertal commented Dec 6, 2024

Let's see if CI hates me for this ... but while researching Histogram issues, I had to try out something that locally worked removing redundant requests:

#203215

@kertal kertal mentioned this issue Dec 6, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience Feature:Discover Discover Application Feature:ES|QL ES|QL related features in Kibana Feature:UnifiedHistogram Issues related to the Unified Histogram plugin impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@kertal @jughosta @elasticmachine @stratoula @davismcphee