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] New field stats in Discover sidebar popover #139072

Merged
merged 122 commits into from
Sep 15, 2022

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Aug 18, 2022

Closes #135886

Summary

This PR continues the work of integrating field stats (as in Lens popover) into Discover sidebar popover. Previous PR: #136328

  • Integrate new <FieldStats /> into Discover
  • Add a new Advanced Setting discover:showLegacyFieldTopValues
  • Change Top Values and histogram colors to blue
  • Show filter buttons next to Top Values terms in Discover only
  • Hide filter buttons for Other top value term
  • Unify Visualize button => now it's in the popover footer
  • Fallback to Discover old logic for non-aggregatable fields (make a separate call to fetch 100 records with existing values for a field)
  • Update percentage calculations? <= we don't have to update calculations as they currently match Field Statistics page
  • Update labels for when analysis is not available
  • Use "Calculated from N sample records"/"Calculated from N records" labels
  • Unify "exist" filter link <= added as an icon button in the popover title
  • Add tests
  • Update Discover tests

Screenshot 2022-09-05 at 14 35 16

Screenshot 2022-09-05 at 14 35 25

Screenshot 2022-09-05 at 14 35 36

Screenshot 2022-09-05 at 14 35 49

Screenshot 2022-09-05 at 14 56 57

Screenshot 2022-09-01 at 16 14 45

Follow up:
#140732
#140733

Checklist

jughosta and others added 30 commits July 13, 2022 16:34
# Conflicts:
#	src/plugins/discover/kibana.json
#	src/plugins/discover/public/build_services.ts
#	x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx
#	x-pack/plugins/lens/server/routes/index.ts
message={i18n.translate(
'unifiedFieldList.fieldStats.notAvailableForThisFieldWithoutDataDescription',
{
defaultMessage: `Analysis is not available for this field.`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ninoslavmiskovic @gchaps Shall we have a separate message for when data is not available? Currently it also shows "Analysis is not available for this field" but we were also discussing bringing "This field is not available for visualizations because it doesn't have any data." copy. I think it would make sense to extend the copy then with "... for this search criteria" or "... based on sample records" but the copy would become too long. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it is implicit, and we can shorten the copy to just say: "This field does not contain any data" ? @gchaps - Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ninoslavmiskovic It can happen that the field has some data but those documents were not a part of the sample. So it would be great to mention why no data: either it's not a part of the sample or it's not present for the current search query/filters.

Copy link
Contributor

@gchaps gchaps Sep 13, 2022

Choose a reason for hiding this comment

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

@jughosta Can you use a different line for each case? For example:

No field data for the current search.
No field data for the current sample.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gchaps Thanks, that works!

Would it be okay to extend the second line to No field data for the current sample of X records.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe we can remove the word current and shorten it a bit:

No field data for X sample records.

@kertal
Copy link
Member

kertal commented Sep 12, 2022

@elasticmachine merge upstream

…in-discover

# Conflicts:
#	src/plugins/discover/public/application/main/components/sidebar/discover_field.test.tsx
#	src/plugins/discover/public/application/main/components/sidebar/discover_field.tsx
#	src/plugins/discover/public/application/main/components/sidebar/discover_field_visualize.tsx
#	src/plugins/discover/public/application/main/components/sidebar/discover_sidebar.tsx
#	x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx
@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream


export const canProvideExamplesForField = (field: DataViewField): boolean => {
if (field.name === '_score') {
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created 2 follow up issues:
#140732
#140733

@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally in Safari, Chrome, Firefox. Great work, it's so much better than our previous implementation! 🥳 All relevant little issues and potential improvements are already in follow up issues. Looking forward to the next steps.

So to merge this it's just the wording of the Advanced settings, then it should be fine

* Side Public License, v 1.
*/

// Adapted from src/plugins/discover/public/application/main/components/sidebar/lib/field_calculator.js
Copy link
Member

Choose a reason for hiding this comment

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

hope we can soon get rid of this, the good first issue didn't work so well 😄

@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 15, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #27 / dashboard app - group 1 dashboard embeddable rendering data rendered correctly when dashboard is hard refreshed

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
unifiedFieldList 75 79 +4

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
discover 78 79 +1
unifiedFieldList 52 59 +7
total +8

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 470.2KB 471.3KB +1.1KB
lens 1.2MB 1.2MB -131.0B
unifiedFieldList 48.4KB 54.3KB +6.0KB
total +6.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
discover 26.3KB 26.5KB +261.0B
unifiedFieldList 6.1KB 6.2KB +56.0B
total +317.0B
Unknown metric groups

API count

id before after diff
discover 95 96 +1
unifiedFieldList 54 61 +7
total +8

ESLint disabled line counts

id before after diff
unifiedFieldList 7 8 +1

Total ESLint disabled count

id before after diff
unifiedFieldList 7 8 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jughosta

@jughosta jughosta merged commit cae3a33 into elastic:main Sep 15, 2022
qn895 added a commit that referenced this pull request Dec 15, 2022
## Summary

This PR removes the beta badge for the Field statistics table. 

<img width="1791" alt="Screen Shot 2022-09-19 at 12 22 30"
src="https://user-images.githubusercontent.com/43350163/191076625-9489eaa0-2488-4a5a-b737-e32724d3bffc.png">

Points of consideration for keeping the beta badge:
- Easier for us to keep collecting more user feedback.
- Potentially switching to [using the new random sampler for aggregation
for the field statistics
table](#138953) in the next
release. Currently, we are pausing this work to match up with the
popover (#139072 and
#140667) and to fine-tune the user
experience/performance.

Points of consideration for removing the beta badge:
- The field stats table has been available to users since 8.1, and has
been in use within ML since 7.x. We should be defining clear criterias
for when it can be moved to GA.

### Checklist

Delete any items that are not applicable to this PR.

- [ ] 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)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] 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))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature
release.

When forming the risk matrix, consider some of the following examples
and how they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces&mdash;unexpected behavior in non-default Kibana Space.
| Low | High | Integration tests will verify that all features are still
supported in non-default Kibana Space and when user switches between
spaces. |
| Multiple nodes&mdash;Elasticsearch polling might have race conditions
when multiple Kibana nodes are polling for the same tasks. | High | Low
| Tasks are idempotent, so executing them multiple times will not result
in logical error, but will degrade performance. To test for this case we
add plenty of unit tests around this logic and document manual testing
procedure. |
| Code should gracefully handle cases when feature X or plugin Y are
disabled. | Medium | High | Unit tests will verify that any feature flag
or plugin combination still results in our service operational. |
| [See more potential risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment Feature:Discover Discover Application release_note:enhancement Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Fieldlist - Refactor field popover stats
10 participants