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

Add label and icon to nested fields in the doc table #54199

Merged
merged 9 commits into from
Jan 15, 2020

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Jan 7, 2020

Summary

Fixes #49842
Depends on #54042

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@Bargs Bargs added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.6.0 labels Jan 7, 2020
@Bargs Bargs requested review from kertal and a team January 7, 2020 22:02
@elasticmachine
Copy link
Contributor

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

// that have the current field in their path. Knowing which fields are nested allows us
// to label them and apply the correct icon in the table row.
const isNestedField = !!indexPattern.fields.find(patternField =>
patternField.subType?.nested?.path.includes(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.

This isn't going to work, it can create false positives. I think I'm going to have to add nested fields to the index pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both ways sounds rather "horrible" :D If we add nested fields to the index pattern, we would make sure they not surprisingly pop up in any editor (I'd assume we give them a special type nested, that we can filter for?). Also I wonder if we need to make some adjustments to the index pattern management UI, so you cannot have an edit page for those, since it wouldn't make any sense?

But triggering false positives sounds also like a horrible idea :D Question for my understanding: Wouldn't the field above not need to match exactly the nested.path of another field? since it should be the nested root itself, we must have that as an exact path somewhere? Wouldn't that also eliminate the false positives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we add nested fields to the index pattern, we would make sure they not surprisingly pop up in any editor

I think as long as the searchable and aggregatable flags are set to false on the field it won't pop up in places where it shouldn't. I checked KQL autocomplete, the filter editor suggestions and the vis editor and they all hide it correctly.

That's a good point about the edit page, I'll look into removing that link for nested fields.

since it should be the nested root itself, we must have that as an exact path somewhere?

There may not be an exact match. Consider a doubly nested field, foo.bar.baz where foo and bar are nested fields and baz is a regular subfield. baz will be the only field in the index pattern, and its path will be foo.bar. But foo will be the key in the flattened doc in Discover, so there won't be an exact match for it.

Copy link
Contributor Author

@Bargs Bargs Jan 8, 2020

Choose a reason for hiding this comment

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

Actually, the edit page includes popularity which does apply to the nested field in Discover since you can add it as a column in the doc table. So I think it makes sense to allow it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we would just need to make sure the edit page does not allow configuring a field formatter? Or rather that no field formatter works for nested fields, so we always have none assigned?

There may not be an exact match. Consider a doubly nested field, foo.bar.baz where foo and bar are nested fields and baz is a regular subfield. baz will be the only field in the index pattern, and its path will be foo.bar. But foo will be the key in the flattened doc in Discover, so there won't be an exact match for it.

I guess I am still a bit confused here :-) Looking at the way we this was shown earlier (#49842) wouldn't foo in that case be the field that will be shown in the column, because we basically "don't go deeper" than the first nested field and from then on just show the list of nested objects? So wouldn't we try to find a match for foo (or foo.bar in case we don't break on the first nested object), but never foo.bar.baz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or rather that no field formatter works for nested fields, so we always have none assigned?

Yeah this is already the case now, the field formatter drop down is there but you can't change it to anything other than "Default". I think it's ok to leave it like that, since someone could technically create a field formatter for nested fields if they wanted to.

So wouldn't we try to find a match for foo (or foo.bar in case we don't break on the first nested object), but never foo.bar.baz?

Right, we'd search for foo because we look no deeper than the first level. The nested path metadata for the foo.bar.baz field will be foo.bar. So foo won't be identified as a nested field if we're only looking for exact matches. But if we look for partial matches we could get false positives on completely unrelated fields, for example if we had another top level field called foofoo it would be identified as a nested field.

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 I understand now, so this will always happen if we have the two nested fields directly below each other. Because saying we would have foo_nested.bar_object.baz_nested.ourfield and would try to look this, we would actually find the foo_nested.bar_object, which has the path metadata set to foo_nested? But as soon as the two nested fields are directly below each other, we're simply missing any field, that would have the link to the outer one?

(Extension with index pattern btw makes anyway sense for me :D)

@Bargs Bargs removed the request for review from kertal January 8, 2020 16:10
@Bargs Bargs requested a review from a team as a code owner January 8, 2020 19:55
@@ -61,6 +62,7 @@ export const typeToEuiIconMap: Partial<Record<string, IconMapEntry>> = {
number: { icon: 'number', color: colors[0] },
_source: { icon: 'editorCodeBlock', color: colors[3] },
string: { icon: 'string', color: colors[4] },
nested: { icon: 'nested', color: colors[4] },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cchaos what color would you recommend here?

Copy link
Contributor

Choose a reason for hiding this comment

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

We're working on coming up with permanent field type icon/color combinations on the EUI side, but for now use colors[2]

@Bargs
Copy link
Contributor Author

Bargs commented Jan 14, 2020

Tests are passing and I added a screenshot showing what it looks like now that EUI 18 is merged. @timroes do we want to put this is 7.6? If so could you give it a final review since you've already been looking at it?

@timroes
Copy link
Contributor

timroes commented Jan 15, 2020

@elasticmachine merge upstream

Copy link
Contributor

@timroes timroes 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 on Chrome Linux behaves as expected. Waiting on green CI

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@timroes timroes merged commit f77b362 into elastic:master Jan 15, 2020
timroes added a commit to timroes/kibana that referenced this pull request Jan 15, 2020
* Apply label and icon to nested fields in the doc table

* Include nested fields in the index pattern so we can identify them in the Discover UI

* use color recommended by design

* Fix unit tests

* Update api integration test

* Fix test comment

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Tim Roes <mail@timroes.de>
timroes pushed a commit that referenced this pull request Jan 15, 2020
* Apply label and icon to nested fields in the doc table

* Include nested fields in the index pattern so we can identify them in the Discover UI

* use color recommended by design

* Fix unit tests

* Update api integration test

* Fix test comment

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Tim Roes <mail@timroes.de>

Co-authored-by: Matt Bargar <mbargar@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 15, 2020
* upstream/master: (72 commits)
  [ML] Calculate model memory limit API integration tests (elastic#54557)
  Skip flakey index template component integration tests. (elastic#54878)
  Add label and icon to nested fields in the doc table (elastic#54199)
  Reverse dependency of home plugin and apm/ml/cloud (elastic#52883)
  [SIEM][Detection Engine] Order JSON keys, fix scripts, update pre-packaged rules
  update invalid snapshot
  add readme note about alerting / manage_api_key cluster privilege (elastic#54639)
  [SIEM] New Overview Page (elastic#54783)
  [Uptime] Feature/refactor context initialization (elastic#54494)
  Upgrade EUI to v18.2.0 (elastic#54786)
  [SIEM] [Detection engine] from signals to timeline (elastic#54769)
  [Index Management] Add Mappings Editor to Index Template Wizard (elastic#47562)
  [SIEM][Detection Engine] Removes deprecated filter from mapping
  [Maps] Add categorical styling (elastic#54408)
  Add mapbox-gl-rtl-text library (elastic#54842)
  [SIEM][Detection Engine] Adds actions to Rule Details (elastic#54828)
  Lexicographically sort location tags (elastic#54832)
  [Maps] expand extent filter to tile boundaries (elastic#54276)
  [Maps] Use v7.6 Elastic Maps Service API (elastic#54399)
  [DOCS] Adds monitoring setting (elastic#54819)
  ...
Bargs pushed a commit to Bargs/kibana that referenced this pull request Jan 15, 2020
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
* Apply label and icon to nested fields in the doc table

* Include nested fields in the index pattern so we can identify them in the Discover UI

* use color recommended by design

* Fix unit tests

* Update api integration test

* Fix test comment

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Tim Roes <mail@timroes.de>
timroes pushed a commit that referenced this pull request Jan 18, 2020
* Revert "Add label and icon to nested fields in the doc table (#54199)"

This reverts commit f77b362

* Apply label and icon to nested fields in the doc table

* Add nested type to field_icon

* Improve nested test and add comment

* Fix tests

* Always pass the field type
timroes pushed a commit to timroes/kibana that referenced this pull request Jan 18, 2020
* Revert "Add label and icon to nested fields in the doc table (elastic#54199)"

This reverts commit f77b362

* Apply label and icon to nested fields in the doc table

* Add nested type to field_icon

* Improve nested test and add comment

* Fix tests

* Always pass the field type
timroes pushed a commit to timroes/kibana that referenced this pull request Jan 18, 2020
* Revert "Add label and icon to nested fields in the doc table (elastic#54199)"

This reverts commit f77b362

* Apply label and icon to nested fields in the doc table

* Add nested type to field_icon

* Improve nested test and add comment

* Fix tests

* Always pass the field type
timroes pushed a commit that referenced this pull request Jan 18, 2020
* Revert "Add label and icon to nested fields in the doc table (#54199)"

This reverts commit f77b362

* Apply label and icon to nested fields in the doc table

* Add nested type to field_icon

* Improve nested test and add comment

* Fix tests

* Always pass the field type

Co-authored-by: Matt Bargar <mbargar@gmail.com>
timroes pushed a commit that referenced this pull request Jan 18, 2020
* Revert "Add label and icon to nested fields in the doc table (#54199)"

This reverts commit f77b362

* Apply label and icon to nested fields in the doc table

* Add nested type to field_icon

* Improve nested test and add comment

* Fix tests

* Always pass the field type

Co-authored-by: Matt Bargar <mbargar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show an icon for nested fields in discover
5 participants