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.
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
[data views] cache field caps requests #168910
[data views] cache field caps requests #168910
Changes from all commits
5dc9eee
797263d
691df4f
609a962
2aec419
2abf10e
4de45b5
8d8fbb4
146ba0b
567e250
830b959
9638c36
442c048
c0e4e26
3f533ab
ffde29e
40704e7
4b02f2d
f294f75
78bd537
86681be
4099571
97e7689
7b7189d
7d93e2b
5de584f
5313afb
521e028
d93f86b
4845511
9dd5d15
7f52130
1566350
abcf7d6
5f123ae
ebc5c67
62e3d79
efdc60a
0b90af3
6cc3ba4
cbfc4de
cde6207
2c0e5d5
e2b2ba7
c929e5f
8f59cf5
7e7da4a
fd083a1
b1e3357
4e5d94d
651a792
8685243
7319b18
45b3dca
4b4266f
2f11c36
5e94706
b0f930e
208f643
e101dba
6e9e871
eb2e989
121c2e8
aa3503b
7734118
b12cade
247bcaa
66836bd
294dfd1
c2d5595
8736f54
387b203
99b03af
4246c60
42bd2f4
fef8a55
6693cf6
7b384d4
486308a
b009721
5c58232
7445428
6ea9098
864d9ef
2d7ab2f
dbee901
80249e5
f26a084
cd4049b
544aa05
df2307e
0fc9422
efa3195
e03a3ec
7bae56d
5c69712
377a5a6
7dc380f
1a9a450
5971b41
fd7adb2
278d005
55b8191
1b8704d
89c4ff2
c0fb816
6efa5f1
9723dc3
c5c993e
6d7212a
ed73b7b
cdeec68
72bbeb7
26d3d6d
7b999db
3a6c239
179105b
3fa1369
f40eb1d
cf7c893
781a580
9b719cc
780fc78
ccd243a
9ec2773
e412172
c51b7d2
6d5df9d
7f7be97
3f7fb79
3c1ab44
fdcd16c
53f1f19
ebd04c7
46c9ee7
60b0675
b563acd
b145375
eb0604d
6216b37
c386791
307a16d
54daa2f
2bce926
af83362
1d0dcd5
797f6ce
913bedb
e6e1e3e
b3349b1
3f58ecd
152d016
08a3ea8
836452a
ca50f4d
b73f727
c7fbf1e
9c4e416
17d4b59
44dded0
999a6a9
1a30c70
dc9fa0e
4b17356
082430d
985ed11
d7701c7
fa237a4
15cc582
3cea884
065a50c
e9fd00e
63907e4
454592c
75cc235
12616b4
27dd198
e0792e6
3a74600
15e5168
139b651
ab514e4
e25dcf9
5cd087f
b424159
9b70dec
30dfec6
f747c2e
9ed0c49
5420c77
f340fae
26c4d72
ab2c743
921a598
ec55f89
c76c87e
165c3cd
cbf3cc9
e4fdc38
07f5058
cbee199
9170300
d62dba7
02a4537
449565e
87d63fe
04e40be
786ce63
a01dd65
f67dc85
603708e
937366b
a6bbc40
15f8771
290a3ca
dda0d98
621a1df
8617b3d
223f546
8549c3f
f751a4e
d024027
e628af7
66fd722
465d993
d18c90a
3a7b331
48d28c9
0a34ba8
a40cc37
c1ab8ff
6342b89
8e5ba7e
b56cb26
19c06b2
3bf8c03
36fc2a6
ff11592
d1d1c0c
14a2fd8
67dbffe
765152d
416a785
9f2700d
194527e
b12b865
99356b3
244896e
a9315ac
0493689
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@kertal Do we need to wait here until it finishes?
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.
yes, else it wouldn't work in Lens (currently), which refreshes the field list, taking the most recent data view, if there are new fields. if we don't wait, we could run into the situation, that fields have not been loaded yet
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.
@kertal We enrich the field list with the new fields from the fields_wildcard response anyway. Why would not they be loaded? And for Lens we do another refresh in datapanel code. I think waiting for all data view fields here conflicts with the purpose of appending new fields manually. How would it improve the performance?
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 need to double check this, because I think when we don't wait for the field result, and this request takes longer, Lens is refreshing the field list using the previous completed data view fields version, while Discover is taking advantage of merging the old with the new fields ... I could be very wrong about his of course. If this wasn't the case, yes, we could think of removing the wait, to get faster results / rendering, while still refreshing the fields in the background
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've checked by removing the
await
of therefreshFields
function, and adding a 5s timeout inside the function to simulate a long running field refresh. In my test, the new field didn't show up. This is high likely because:kibana/x-pack/plugins/lens/public/datasources/form_based/datapanel.tsx
Line 301 in d024027
kibana/x-pack/plugins/lens/public/datasources/form_based/datapanel.tsx
Line 283 in d024027
refreshFields
function hasn't completed, new fields don't show up... and I tested in Discover, where the field was added unmapped, for high likely another reason
Would it be possible to remove the await for refreshFields, in theory yes, but it would need definitely changes (something we can do further down the road)
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.
@kertal Thanks for looking into it!
We could update the code where it separates Unmapped fields https://github.com/elastic/kibana/blob/main/packages/kbn-unified-field-list/src/hooks/use_grouped_fields.ts#L159-L161 to account for
newFields
in Discover.The issue is that making the UnifiedFieldList know about new fields is not enough, data view instance needs to know about them too as we call
dataView.getFieldByName
anddataView.fields
in other places. Callingawait dataViewsService.refreshFields(dataView, false, true);
"fixes" the data view. But it also extends loading time.Probably it's fine for now as our main objective is to automatically show new fields.
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 think we can optimize that, but since it's just refreshed when there are new fields it's not generally slowing the loading for other cases. So I don't think it's a priority (But a nice to have ... of course)
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 also agree this should be fine for this PR, although I'm open to exploring potential improvements as a followup, especially where @jughosta has the best understanding of Unified Field List.