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

[Lens] show meta field data in Lens #77210

Merged
merged 17 commits into from
Sep 24, 2020

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Sep 10, 2020

Fixes #75632

This PR makes sure meta fields are also handled correctly in terms of existence data and stats.

Screenshot 2020-09-15 at 13 52 20

The biggest change here is to pass down the list of meta fields from advanced settings to the existence checker and look them up directly on the document instead of _source or fields.

Also, the meta flag is made part of the field type to introduce a new "Meta fields" section in the field list.

There are two related issues due to buggy handling of meta fields in Kibana in general which also show up here:

  • The id field is showing up but throws an error when trying to aggregate it. This is not a Lens bug and has to be solved on index pattern level (or maybe ES level) Index pattern: "_id" field marked as aggregatable #77206
  • _type is showing up as empty. This is not a Lens bug as well, IMHO the field shouldn't even be present in the index pattern in the first place. Again, this is either an index pattern bug or an ES bug (didn't dig into it): Discover: "_type" showing up as empty #77209 It should work fine on 7.x

@cchaos
Copy link
Contributor

cchaos commented Sep 10, 2020

Should meta fields be indicated in the field list somehow?

Maybe a different section like the empty fields?

@flash1293
Copy link
Contributor Author

flash1293 commented Sep 10, 2020

Maybe a different section like the empty fields?

I can definitely see a separate section in the very bottom (as they are an edge case anyway), with using the color to indicate emptiness (instead of having both “available meta fields” and “empty meta fields” sections, that seems like overkill). I will adjust the PR

@flash1293 flash1293 marked this pull request as ready for review September 15, 2020 11:54
@flash1293 flash1293 requested a review from a team September 15, 2020 11:54
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Sep 15, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Tested on FF and all works well. The code looks good. Just one note that is not even related to this PR - the last element of the accordion is cut (you didn't introduce it though):

Removing the last 0 from these styles solves the problem, but as it's not related to this PR, feel free to ignore this comment :)
image

@flash1293
Copy link
Contributor Author

@mbondyra Refactored the different sections into a FieldList component if you want to take another look

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 To the separated section. This way the user can mostly just ignore those altogether.

A few questions/feedback:

  1. Should we allow users to drag-n-drop these or select them to be aggregated at all? Some of them completely error and others are most likely not unique at all.

  2. Can you also update the organization within the field selector combo box?

Screen Shot 2020-09-16 at 13 12 35 PM

  1. There's seems to be a discrepency between the messaging and the look of the _id field. It shows as if it has data (white bg) but the popover says it doesn't have any.

Screen Shot 2020-09-16 at 13 10 43 PM

  1. Can we update the information icon tooltips to have better messaging per category of field?

Empty fields
Screen Shot 2020-09-16 at 13 17 13 PM

Tooltip:

This field doesn't have any data but you can still drag and drop to visualize.

Popover:

This field is empty because it doesn’t exist in the 500 sampled documents. Adding this field to the configuration may result in a blank chart.

Meta fields
Screen Shot 2020-09-16 at 13 20 06 PM

The messaging will depend on if we decide they can still add these to the chart, but if we say they can't then:

Tooltip:

Meta fields cannot be visualized. Click for a field preview.


return (
<div
className="lnsInnerIndexPatternDataPanel__listWrapper"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget now to update class names to align with the component name and move the SASS to be imported directly by the JS component.

@flash1293
Copy link
Contributor Author

flash1293 commented Sep 17, 2020

Thanks for taking a look, @cchaos

Should we allow users to drag-n-drop these or select them to be aggregated at all? Some of them completely error and others are most likely not unique at all.

There are currently problems with _id and _type (see linked issues in the PR description, they are out of scope for this PR, but should get addressed), but in general aggregating meta fields, while being an edge case, can be useful and is supported in the rest of Kibana as well - I think we should support them (and we already do today, although in a buggy way).

Can you also update the organization within the field selector combo box?

Great point, fixed that.

There's seems to be a discrepency between the messaging and the look of the _id field. It shows as if it has data (white bg) but the popover says it doesn't have any.

_id is currently broken unrelated to this PR (see PR description)

Can we update the information icon tooltips to have better messaging per category of field?

Makes sense, done

The messaging will depend on if we decide they can still add these to the chart, but if we say they can't then:

As mentioned they should be addable to the chart, everything else is a bug. To give a bit of context here, there are also plugins adding additional meta fields which are supported in Kibana used from time to time - those might be more helpful than _id and _type (_index is already a good example of a useful meta field)

Don't forget now to update class names to align with the component name and move the SASS to be imported directly by the JS component.

Thanks, moved the SASS over

@flash1293 flash1293 requested a review from a team as a code owner September 17, 2020 11:42
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Initial testing shows that there is a large performance regression with large index patterns when interacting with the filters, compared to the previous behavior.

@flash1293
Copy link
Contributor Author

Ah, I planned to check whether memoization and so on still works after the refactoring and forgot. I will take a look, thanks for taking performance into account 👍

} else if (containsData(field)) {
return 'availableFields';
} else return 'emptyFields';
}),
};
}, [allFields, existingFields, currentIndexPattern, hasSyncedExistingFields]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I traced the performance, and the main issue is that this first set of memoized code is expensive to run, but is now running on each keystroke.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @flash1293

@flash1293
Copy link
Contributor Author

@wylieconlon Improved the performance of field group calculation by splitting out the filtering in a separate hook like it was done before.

@flash1293
Copy link
Contributor Author

While looking into performance, I noticed a pretty large performance bottleneck which was super easy to remove: 7ee7bbb

We were building a field index by name for the whole field list on each containsData check (which is running for each single field).

It looks like this would close #77898 as well

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
lens 471 +10 461

page load bundle size

id value diff baseline
lens 1.1MB +7.1KB 1.1MB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] The system fields are shown as empty
6 participants