-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Lens] accuracy mode for Top Values #129220
[Lens] accuracy mode for Top Values #129220
Conversation
c1deba5
to
7553381
Compare
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx
Show resolved
Hide resolved
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…te/kibana into f/accuracy-mode-for-top-values
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
@flash1293 what is the official process for removing a translation string that is no longer used? |
While testing this I noticed that the multi terms agg is actually not reporting precision errors - it's missing this implementation:
Since you are working on this anyway right now, could you add it? |
@elasticmachine merge upstream |
Here are some suggestions for the copy in the tooltip and the warning: Tooltip Improves results for high-cardinality data, but increases the load on the Elasticsearch cluster. Warning 1 Top 5 values for customer_first_name.keyword might be an approximation. Enable accuracy mode (use sentence case) Warning 2 Top 5 values for customer_first_name.keyword might be an approximation. |
x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts
Outdated
Show resolved
Hide resolved
@gchaps thanks for those suggestions. Way better than what I had. |
@andrewctate In the updated screenshots, "top 5 values" is repeated in the text. Is that just for the example you are showing? I'm not sure where {name} is coming from. |
@gchaps thanks for catching that 🤦♂️ . Here's the real deal: |
💚 Build SucceededMetrics [docs]Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Text LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app services code LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again and works well now even for multi field terms, LGTM 👍
Summary
Resolves #117909
Screen.Recording.2022-04-01.at.3.46.22.PM.mov
Help icon:
Checklist
Delete any items that are not applicable to this PR.