-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Use index pattern service instead saved object client #74654
[Lens] Use index pattern service instead saved object client #74654
Conversation
18dc6b6
to
78e42e9
Compare
Pinging @elastic/kibana-app (Team:KibanaApp) |
@elasticmachine merge upstream |
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.
I tested this in Chrome and it works great with regular index patterns, but I can't use it with rollups at all. I know there are some problems in master as well, but it seems like on this PR the fields are not shown (not sure why)
It's probably related to the IndexPatternField
instance which has some methods along with the regular JSON properties. Maybe we can get around it by not spreading the field in here https://github.com/elastic/kibana/pull/74654/files#diff-2edf5b6f299eb91ac9ef5fd386eef219R72 but specifically copying over the values we actually use. Not sure though, maybe there's a big reason to use the actual IndexPatternField
stuff I'm missing.
Also, I know this wasn't introduced by this PR, but the IndexPattern
objects we use in Lens aren't actually IndexPattern
s. It's basically this imprecise cast: https://github.com/elastic/kibana/pull/74654/files#diff-2edf5b6f299eb91ac9ef5fd386eef219R80 If it's a lot of work, please omit, but if it's easy to add, we could introduce our own type here which is actually correct (maybe just by letting type inference do it's job and using the return value of the loader).
@flash1293 thanks for checking - I tested the rollup indices before marking this PR as ready, but I apparently had a snippet added in my code for testing displayName and it had hidden the bug. Sorry about it! |
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 in Chrome and Firefox and works fine for me - dotted field names are used correctly.
Search in the data panel works fine for both ways of writing the field name which is very helpful I think - in the field dropdown in the dimension popover however on the display name is considered. We can't change this for now because EUI is controlling the matcher (elastic/eui#2199). However I think the behavior is fine for now.
Added a typing nit, but this looks good to me overall
const indexPatterns = await Promise.all(missingIds.map((id) => indexPatternsService.get(id))); | ||
const indexPatternsObject = indexPatterns.reduce( | ||
(acc, indexPattern) => { | ||
const newFields = (indexPattern.fields as IndexPatternField[]) |
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.
This cast is not precise and can be omitted
aggregatable: field.aggregatable, | ||
scripted: field.scripted, | ||
searchable: field.searchable, | ||
aggregationRestrictions: field.aggregationRestrictions, |
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.
If you remove the case from above, this line will error because the field of an IndexPattern
instance doesn't have aggregationRestrictions
. You can simply set this to an empty object, because to logic below will map the restrictions into the fields.
…exPatternFromIPService # Conflicts: # x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Just tested combined with #70039, LGTM
💚 Build SucceededBuild metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
* master: (71 commits) [Lens] Show 'No data for this field' for empty field in accordion (elastic#73772) Skip failing lens test Configure ScopedHistory consistenty regardless of URL used to mount app (elastic#75074) Fix returned payload by "search" usage collector (elastic#75340) [Security Solution] Fix missing key error (elastic#75576) Upgrade EUI to v27.4.1 (elastic#75240) Update datasets UI copy to data streams (elastic#75618) [Lens] Register saved object references (elastic#74523) [DOCS] Update links to Beats documentation (elastic#70380) [Enterprise Search] Convert our `public_url` route to `config_data` and collect initialAppData (elastic#75616) [Usage Collection Schemas] Remove Legacy entries (elastic#75652) [Dashboard First] Lens Originating App Breadcrumb (elastic#75470) Improve login UI error message. (elastic#75642) [Security Solution] modify circular deps checker to output images of circular deps graphs (elastic#75579) [Data Telemetry] Add index pattern to identify "meow" attacks (elastic#75163) Migrate CSP usage collector to `kibana_usage_collection` plugin (elastic#75536) [Console] Get ES Config from core (elastic#75406) [Uptime] Add delay in telemetry test (elastic#75162) [Lens] Use index pattern service instead saved object client (elastic#74654) Embeddable input (elastic#73033) ...
Summary
Uses indexPatternsService provided by the data plugin to fetch index patterns instead of saved object client.
Fixes #73306
Uses displayName field property as labels instead of raw field.name.
Field displayName changes
To test it, you can add this snippet to 58. line of x-pack/plugins/lens/public/indexpattern_datasource/loader.ts:
Checklist