[discover] use field caps api to load field list #149294
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ttkime/kibana into discover_field_list_load_improvements
mattkime
commented
Jan 31, 2023
| isPlainRecord: isPlainRecordType, | ||
| }, | ||
| }); | ||
| if (!isPlainRecordType || !props.useNewFieldsApi) { |
There was a problem hiding this comment.
I should be able to remove these props.useNewFieldsApi checks - I think I mixed them up with another config setting.
mattkime
commented
Jan 31, 2023
| dispatchSidebarStateAction({ | ||
| type: DiscoverSidebarReducerActionType.DOCUMENTS_LOADED, | ||
| payload: { | ||
| dataView: selectedDataViewRef.current, |
There was a problem hiding this comment.
I'm pretty sure I want to use selectedDataViewRef.current for all data view refs BUT when I look at the current code, I get a different value before and after loadFieldExisting is fired
kertal
reviewed
Feb 1, 2023
| @@ -657,6 +657,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { | |||
| await testSubjects.missingOrFail('discoverNoResultsError'); | |||
| }); | |||
|
|
|||
| // run this test only | |||
There was a problem hiding this comment.
this failure is high likely caused by the unmapped fields.
Adding some debugs here showed
Error: expected '0 available fields. 46 unmapped fields. 7 empty fields. 3 meta fields.' to equal '0 available fields. 7 empty fields. 3 meta fields.'
it's the 46 unmapped fields caused it to failed. And those are high likely mapped fields of the previous data view. when those are cleaned up or the unmapped fields are overwritten when the new incoming data view fields are available, it should work
⏳ Build in-progress, with failuresFailed CI Steps
Test Failures
History
To update your PR or re-run it, just comment with: |
1 task
|
Closing since it won't work due to limitations in Elasticsearch: elastic/elasticsearch#94888 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #145796
tldr;
documents$.subscribeis not in sync with component props, therefore I can't get the current data view when a request to load documents is fired.My current focus - I'm running the single highlighted functional test in
_sidebar- then I switch betweenwith-timestampandlogstash-*data views.with-timestamphas about 7 fields andlogstash-*about 10x that and I use that as a guide as to whether the sidebar is showing correct info. Switching between the two will frequently result in the wrong fields being displayed. After much poking around, I discovered thatdocuments$fires off the request to load documents before the props have been updated onDiscoverSidebarResponsivewhich means I don't yet have access to the correct data view object. The typical result is that it shows the fields for the previously selected data view. If you look in the console you'll see comments noted 1, 2, and 3 which trace the initial data view change, the component prop changes, and the call to load fields which are disappointingly out of sync.I took a look at adding the current data view to the
documents$emitted values but complexity appears to grow in that direction. Field loading is triggered by thedocuments$loading state. This also appears to be used forresetcode which is disconnected from any context. Its probably possible to make this work but I haven't yet found the bounds of the problem.The observable / params sync issue exists in current code but is inconsequential.
One potential solution is to use
useDiscoverStateas its has centralized state. I haven't yet investigated how practical this is.Checklist
Delete any items that are not applicable to this PR.
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:
For maintainers