Closed Kerry350 closed 17 hours ago
Kerry350
commented
4 days ago @flash1293
Thanks for taking a look 👌
When viewing mapped/inherited fields, it gives me an error in the flyout:
This is due to the note in the PR description about the bug (or what I think is a bug) in the Elasticsearch JS library. I did drop a message in the dev tools channel, but I haven't heard back. I'll ping again. You can throw that hacky change in to get things working locally.
flash1293
commented
4 days ago This is due to the note in the PR description about the bug (or what I think is a bug) in the Elasticsearch JS library. I did drop a message in the dev tools channel, but I haven't heard back. I'll ping again. You can throw that hacky change in to get things working locally.
Ah got it, thanks.
Runtime mappings don't seem to work with match_only_text: mapper_parsing_exception: No handler for type [match_only_text]
I think that's expected, maybe we can go with keyword and a high ignore_above here? Or just read the value from _source ?
Kerry350
commented
2 days ago @flash1293 This is ready for another look.
The Elasticsearch JS lib error is being looked at by Josh Mock.
I think that's expected, maybe we can go with keyword and a high ignore_above here? Or just read the value from _source ?
I've made this a keyword for now. Although ignore_above doesn't seem to be compatible with runtime mappings.
Everything else is changed 👍
flash1293
commented
2 days ago Thanks for the latest round of changes, @Kerry350 !
Some notes:
When testing I noticed that with the small sample of 20 docs it's very easy to run into the case of "couldn't find the field you try to map in the sample". What about adding it as a keyword runtime field with an exists query when fetching the sample docs so we have a more representative set of documents? Independent of that I think we can increase the number of sampled docs to 200 or so
There is a lot of spacing between the form and the preview table, could we remove that?
Seems like the issue with the search bar scrolling out of view still exists
(can happen on a follow-up PR) - I noticed that it would be nice to have a refresh button for the schema editor, as new unmapped fields could come in. Not a blocker though
Kerry350
commented
2 days ago @flash1293
When testing I noticed that with the small sample of 20 docs it's very easy to run into the case of "couldn't find the field you try to map in the sample". What about adding it as a keyword runtime field with an exists query when fetching the sample docs so we have a more representative set of documents? Independent of that I think we can increase the number of sampled docs to 200 or so
Yeah, that's a very fair point. I'll up it and add the exists, and trim to 20 on the UI. Performance does seem to be quite poor for using date with a format and runtime mappings, but at least it's only one case.
There is a lot of spacing between the form and the preview table, could we remove that?
👍
Seems like the issue with the search bar scrolling out of view still exists
Hmm, out of interest are you seeing this all the time, or only when you resize the screen whilst mounted? If the latter I think it's because the Data Grid might not be recalculating dimensions on resize somewhere. There's a lot of elements with overflow: auto higher in the hierarchy, but I thought I'd tweaked the right elements here 😅 I'll take another look.
(can happen on a follow-up PR) - I noticed that it would be nice to have a refresh button for the schema editor, as new unmapped fields could come in. Not a blocker though
Sounds good. There's a few refinements / followups that can go in one issue / PR.
Kerry350
commented
2 days ago @flash1293
This should be good, I've added the simulation changes etc.
Regarding the search bar it does seem to be that the data grid isn't amending it's container height on resize. I don't see this issue when loading the page initially, only if I resize the window after loading / mounting the grid. Do you think that's okay for iteration one, and okay for a followup fix?
elasticmachine
commented
1 day ago Fewer modules leads to a faster build time
| id | before | after | diff |
|---|---|---|---|
streamsApp |
152 | 168 | +16 |
Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app
| id | before | after | diff |
|---|---|---|---|
streamsApp |
96.2KB | 115.0KB | +18.8KB |
cc @Kerry350
kibanamachine
commented
17 hours ago Starting backport for target branches: 8.x
kibanamachine
commented
17 hours ago | Status | Branch | Result |
|---|---|---|
| ✅ | 8.x |  |
Note: Successful backport PRs will be merged automatically after passing CI.
Please refer to the Backport tool documentation
Summary
Implements https://github.com/elastic/observability-dev/issues/4133.
Opening this up for a first pass as the PR is getting quite big. I've listed below some things that can be improved in further iterations.
High level notes
formathas been added to the field definitionFollowups
We could potentially use a separate API for the mapping edits, rather than the core edit route, to be more performant, but for now this is used to create less surface area / deviation.
State management is rudimentary right now. It could be improved with a
useReducerapproach to avoid potentialuseStaterace conditions, and then even something like xstate when things are more concrete. No state syncs with the URL currently.We could provide a lot more assistance with
format. We could provide a dropdown with options, and then a toggle to do custom. (Actually, it looks like in the refined designs this is a dropdown, so I'll probably switch this to a select with predefined options)Issues
simulate.ingestdon't work asbodyis just set toundefined(chasing this up). You can do the following patch in node_modules just to get things going (runyarn startagain):match_only_text:mapper_parsing_exception: No handler for type [match_only_text]Open questions
Screenshots