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] Redundant data view fields request when switching data views #167221

Closed
Tracked by #167735 ...
kertal opened this issue Sep 26, 2023 · 11 comments
Closed
Tracked by #167735 ...

[Discover] Redundant data view fields request when switching data views #167221

kertal opened this issue Sep 26, 2023 · 11 comments
Assignees
Labels
bug Fixes for quality problems that affect the customer experience :DataDiscovery/fix-it-week Feature:Discover Discover Application impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.

Comments

@kertal
Copy link
Member

kertal commented Sep 26, 2023

Kibana version:
Main, 8.10.x

Describe the bug:
When switching data views there are 3 requests to the _fields_for_wildcard API, instead of 2

Steps to reproduce:

  1. Go to Discover
  2. Switch data view
  3. In the network tab of your browser there are 3 requests

Expected behavior:
There should be just 2 requests
Screenshots (if relevant):
Discover_-_Elastic

@kertal kertal added bug Fixes for quality problems that affect the customer experience Feature:Discover Discover Application loe:small Small Level of Effort Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Sep 26, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@kertal kertal added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. :DataDiscovery/fix-it-week labels Sep 26, 2023
@lukasolson
Copy link
Member

Possibly related: #162403

@kertal kertal added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. and removed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Oct 2, 2023
@kertal
Copy link
Member Author

kertal commented Oct 3, 2023

I think this is happening because the data_view_picker fetches the fields of the currently selected data view, when the data view is switched

onChangeDataView={async (newId) => {
try {
// refreshing the field list
await dataViews.get(newId, false, true);
} catch (e) {
//
}
setSelectedDataViewId(newId);
setPopoverIsOpen(false);
if (isTextBasedLangSelected && !isTextLangTransitionModalDismissed) {
setIsTextLangTransitionModalVisible(true);
} else if (isTextBasedLangSelected && isTextLangTransitionModalDismissed) {
setIsTextBasedLangSelected(false);
// clean up the Text based language query
onTextLangQuerySubmit?.({
language: 'kuery',
query: '',
});
onChangeDataView(newId);
setTriggerLabel(trigger.label);
} else {
onChangeDataView(newId);
}

and the callback function of the component is given the actual id of dataView (onChangeDataView(newId))

Then the consuming application, in this case Discover, uses the id to fetch the DataView and it's fields again.

nextDataView = typeof id === 'string' ? await dataViews.get(id, false) : id;

this is redundant, and could be resolved by passing in the data view object on the callback function

onChangeDataView(dataViewObj)

then the consumer can use the the dataViewObj, and doesn't need to trigger another operation fetching the fields again, which causes the redundancy described in this issue

@jughosta
Copy link
Contributor

jughosta commented Oct 4, 2023

I think the second request is happening because of this parameter #160195

@kertal
Copy link
Member Author

kertal commented Oct 4, 2023

I think the second request is happening because of this parameter #160195

but I think is just should be triggered when entering Discover, not when switching data views? (which does not mean it shouldn't also be consolidated to reduce requests)

@mattkime
Copy link
Contributor

mattkime commented Oct 8, 2023

A while back...unfortunately I'm not sure how far back...data views seemed to be refreshed whenever the user switched kibana apps. Perhaps this relied on an idiosyncrasy of the kibana app lifecycle that no longer exists. Perhaps it never really worked that way and I was simply mistaken. Whatever the case, they clearly don't work that way now.

Initially we saw new fields as a rare case. You had to push a button to pick up new fields. The button was removed, but not without debate as to whether it should be left, simply devoid of actual function. Well, on occasion new fields are exactly what people are working on and want to see and they complain when they don't - #157239

Then we decided its best to get fresh fields whenever we load a data view - #160195 - generally speaking, this is very effective. Unfortunately when field lists are slow, this makes them much slower. We've made the field list update more frequently at the expense of performance under specific conditions.

This is when discover started refreshing the field list although its unclear why - #136256

...anyways, I need to put this down for a bit. I'll come back to this and see if I arrive at either a conclusion or a well defined paradox.

@kertal
Copy link
Member Author

kertal commented Oct 9, 2023

Then we decided its best to get fresh fields whenever we load a data view - #160195 - generally speaking, this is very effective. Unfortunately when field lists are slow, this makes them much slower. We've made the field list update more frequently at the expense of performance under specific conditions.

Thx for the research 👍 We might think of discussing why the field list needs to be refreshed and if it wouldn't be better to allow the user to do this, if needed. This would prevent a lot of redundant fetching, improving the performance.

I was having another look at #160195, you're both right, this is the cause for the redundant request when switching data views in the UI. ChangeDataView is refreshing the fields, and then it's done again when resolving the selected data view in Discover. We might consider to revert this change, to find a better solution, e.g. we could pass down configuration to ChangeDataView not to refresh the fields (and enable this in Discover), or something similar, reducing redundant requests.

@kertal
Copy link
Member Author

kertal commented Oct 9, 2023

@jughosta we are currently requesting the filtered fields in the UnifiedFieldList to distinguish between available fields and "empty" fields, which is done here, right?

export function existingFields(filteredFields: FieldSpec[], allFields: Field[]): string[] {
const filteredFieldsSet = new Set(filteredFields.map((f) => f.name));
return allFields
.filter((field) => field.isScript || field.runtimeField || filteredFieldsSet.has(field.name))
.map((f) => f.name);
}

Couldn't we use those filtered fields, in case there are new fields, to add those to the displayed fields?

@jughosta
Copy link
Contributor

jughosta commented Oct 9, 2023

@kertal Yes, we might be able to add new fields or replace properties of the existing ones with this approach. But not to hide former fields.

@kertal
Copy link
Member Author

kertal commented Oct 9, 2023

@jughosta thx, so former fields that we couldn't hide, these would be e.g. if there were indices deleted and no other indices available with this fields, right? Those would then become empty, which IMO would be an acceptable trade off for reducing _field_caps requests. Also in the UnifiedFieldList, we could offer UI to refresh the fieldlist by the user, when needed

mattkime added a commit that referenced this issue Dec 1, 2023
## Summary

The data view picker reloads the field list when the data view is
changed. Discover also does this. It doesn't need to happen twice so
this removes the discover field list reload call.

Addresses #167221
@kertal
Copy link
Member Author

kertal commented Dec 1, 2023

Fixed by #168315

@kertal kertal closed this as completed Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience :DataDiscovery/fix-it-week Feature:Discover Discover Application impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
Development

No branches or pull requests

5 participants