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

[UnifiedFieldList] Add new fields ingested in background with valid mappings #172329

Merged
merged 45 commits into from
Jan 9, 2024

Conversation

kertal
Copy link
Member

@kertal kertal commented Nov 30, 2023

Summary

This PR improves UnifiedFieldList adding a newly ingested field to be added to the list with already the right type in Discover. On top of that, it triggers a refresh of the selected DataView fields, so the new field will be available by consumers of the DataView, no reload is necessary, and also when we introduce caching, the cache will be refreshed. One more benefit, when switching data views, the field refreshing was removed. It's no longer necessary, making data view switching snappy, while the UnifiedFieldList makes sure, new fields are showing up, and the data view field cache is refreshed behind the scenes, if necessary.

How to test

In Console ingest a new record

PUT field-refresh-test/_doc/1
{
  "message": "Hi"
}

Go to Discover and create a DataView for field-refresh-test

Open Kibana / Console in a second tab/window and ingest

PUT field-refresh-test/_doc/1
{
  "message": "Hi again!",
  "🐣": "🐔"
}

Refresh the search in the first window. Then you should be able to see the new field with the correct mapping in the field list.

Bildschirmfoto 2023-12-01 um 09 49 03

Before this change the new field was available, but unmapped. It also works in Lens, when new fields caps are requested due to change of query / filters / time range

Resolves #172342
Resolves #169360
Resolves #170874

Limitation: A type change won't be detected (this could be a follow up task)

Checklist

@kertal kertal self-assigned this Nov 30, 2023
@kertal kertal added Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. enhancement New value added to drive a business result labels Dec 1, 2023
@kertal
Copy link
Member Author

kertal commented Dec 4, 2023

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Dec 7, 2023

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Dec 13, 2023

/ci

@kertal
Copy link
Member Author

kertal commented Dec 13, 2023

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Dec 13, 2023

/ci

@kertal kertal changed the title [UnifiedFieldList] Add new fields to list with correct types [UnifiedFieldList] Add new fields ingested in background with valid mappings Dec 14, 2023
@@ -245,6 +230,27 @@ export const UnifiedFieldListSidebarComponent: React.FC<UnifiedFieldListSidebarP
onOverrideFieldGroupDetails: stateService.creationOptions.onOverrideFieldGroupDetails,
});

useEffect(() => {
Copy link
Member Author

@kertal kertal Dec 14, 2023

Choose a reason for hiding this comment

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

note, this code was just moved to have access to the new modified list of allFields

@kertal kertal requested a review from mattkime December 14, 2023 21:55
@kertal
Copy link
Member Author

kertal commented Dec 15, 2023

/ci

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Seems to work well in Lens.

Related issues #149093, #143877

@jughosta
Copy link
Contributor

jughosta commented Jan 3, 2024

Reviewed and tested the PR again today. I found some issues and addressed them via kertal#16

@davismcphee Would be great if you could review it too.

…s-updated

[Discover] Small fixes for types and memo deps. Add tests.
@kertal
Copy link
Member Author

kertal commented Jan 8, 2024

@elasticmachine merge upstream

@kertal kertal requested a review from davismcphee January 8, 2024 00:40
@kertal
Copy link
Member Author

kertal commented Jan 8, 2024

@elasticmachine merge upstream

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Code changes look good, and it worked great in testing in both Discover and Lens! I even tried to fool it a bit by deleting and recreating the index, adding different fields, etc., but the behaviour seemed good in each case (old fields after deleting the index got moved to empty fields, etc.).

It's a valuable enhancement with or without caching, but especially useful considering the upcoming changes. I left a couple of minor code suggestions, but nothing I'd consider blocking. Thanks and LGTM!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #7 / RegisteredAttachmentsPropertyActions does show the property actions with only delete permissions
  • [job] [logs] Jest Tests #7 / RegisteredAttachmentsPropertyActions remove attachments correctly
  • [job] [logs] Jest Tests #7 / RegisteredAttachmentsPropertyActions renders correctly registered attachments
  • [job] [logs] Jest Tests #7 / RegisteredAttachmentsPropertyActions renders the correct number of actions
  • [job] [logs] Jest Tests #7 / RegisteredAttachmentsPropertyActions renders the modal info correctly

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 688 689 +1
eventAnnotationListing 461 462 +1
lens 1191 1193 +2
logExplorer 637 638 +1
total +5

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/unified-field-list 261 267 +6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 555.3KB 556.3KB +1.1KB
eventAnnotationListing 196.6KB 196.9KB +320.0B
lens 1.4MB 1.4MB +1.4KB
unifiedSearch 213.8KB 213.7KB -35.0B
total +2.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
discover 31.4KB 31.5KB +139.0B
Unknown metric groups

API count

id before after diff
@kbn/unified-field-list 285 291 +6

History

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

cc @kertal

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Another quick review after the latest round of changes, and everything is still working as intended. LGTM, thanks!

@kertal kertal merged commit 1d8eb89 into elastic:main Jan 9, 2024
21 checks passed
@kibanamachine kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 9, 2024
@mattkime
Copy link
Contributor

mattkime commented Jan 9, 2024

Code looks good, everything seems to be working well. Nice work!

nreese pushed a commit to nreese/kibana that referenced this pull request Jan 10, 2024
…appings (elastic#172329)

Improves UnifiedFieldList by adding a newly ingested field to the list with the right type. On top of
that, it triggers a refresh of the selected DataView fields, so the new field be available by consumers of the DataView.

Co-authored-by: Julia Rechkunova <[email protected]>
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
…appings (elastic#172329)

Improves UnifiedFieldList by adding a newly ingested field to the list with the right type. On top of
that, it triggers a refresh of the selected DataView fields, so the new field be available by consumers of the DataView.

Co-authored-by: Julia Rechkunova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting enhancement New value added to drive a business result Feature:Discover Discover Application release_note:enhancement Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.13.0
Projects
None yet
8 participants