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

[Discover] Move total count logic from UnifiedHistogram to Discover main #177156

Open
jughosta opened this issue Feb 19, 2024 · 3 comments
Open
Labels
Feature:Discover Discover Application impact:medium Addressing this issue will have a medium 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. technical debt Improvement of the software architecture and operational architecture

Comments

@jughosta
Copy link
Contributor

jughosta commented Feb 19, 2024

Currently, Discover passes filters, query and time range to UnifiedHistogram component and UnifiedHistogram component reports back the total count of documents matching the query.

If chart is visible, UnifiedHistogram fetches chart data and derives total count from that request. In some ES|QL cases, Discover passes table with already fetched results and UnifiedHistogram derives total count from that passed data(outdated, Discover takes the number of fetched rows). Then it reports the result back to Discover.

If chart is hidden, UnifiedHistogram makes a request to fetch only total count for data view mode. Then it reports the result back to Discover.

Also there are issues with having redundant requests from UnifiedHistogram #165192

The logic is getting complicated and seems like that this back and forth between UnifiedHistogram and Discover can become a problem of reporting an incorrect total hits count in the future. During the development I ran into an issue once (time range was not updated in UnifiedHistogram correctly) when 0 was coming from UnifiedHistogram and it was triggering "No results" screen in Discover although Discover request had results.

So it's important to make sure that the same filters, query and time range are used both for Discover request and for total hits count request. We can achieve this by moving the logic of deriving total hits count out of the UnifiedHistogram and closer to the the Discover main request (as a separate _count request or with track_total_hits). And we should not use component props for passing filters, query and time range and relying on React hooks.

@jughosta jughosta added 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. labels Feb 19, 2024
@elasticmachine
Copy link
Contributor

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

@jughosta
Copy link
Contributor Author

jughosta commented Feb 22, 2024

@kertal kertal added impact:medium Addressing this issue will have a medium 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 technical debt Improvement of the software architecture and operational architecture labels Mar 14, 2024
jughosta added a commit that referenced this issue Mar 25, 2024
…e number of fetched records instead. (#178711)

- Addresses a part of #177156

## Summary

This PR changes how the total hits counter is reported in Discover for
ES|QL mode. The previous logic could be misleading as the received
documents count sometimes was not the same as the shown total hits
counter. Often it's because for ES|QL queries without sorting, separate
requests might return different result.

Before:
Number of received docs in Discover was considered only as "partial"
result for total hits. The final value was coming from UnifiedHistogram:
- if chart is visible and histogram => total hits are based on the
result of the histogram fetch request
- if chart is visible and it's not a histogram but based on discover
fetched records => total hits are calculated from the number of records
- if chart is hidden => total hits are based on the results of the
separate request for getting the count only

After:
Number of received docs in Discover is considered as "complete" result
for total hits. Changes for UnifiedHistogram:
- if chart is visible and histogram => logic stays but the histogram
total hits count is ignored on Discover side
- if chart is visible and it's not a histogram but based on discover
fetched records => logic stays but the histogram total hits count is
ignored on Discover side
- if chart is hidden => this request is removed completely.

It's a proposal. Happy to discuss.

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
@jughosta
Copy link
Contributor Author

jughosta commented Oct 31, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application impact:medium Addressing this issue will have a medium 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. technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

3 participants