[Streams] [Dedicated Grok UI] Package / highlight matched parts / suggestions

[Kerry350]

Summary

Closes https://github.com/elastic/streams-program/issues/170 and https://github.com/elastic/streams-program/issues/171

Reviewer notes

It's worth reading the research in https://github.com/elastic/streams-program/issues/168 to understand why certain decisions were made. Some of the resolving code is inspired by grok-js, unfortunately this wasn't something we could use directly.

The design / UX is not final. This is just a functional version. The editor is currently on the streams > enrichment page so it can be played with, this will not be merged.

There is pattern support for our ECS patterns, I have not added the legacy ones.

There will almost certainly be some edge cases that don't work, every repo for a Grok tool I looked at had their own 😅 I've tried to test this with lots of varied examples though.

I'd recommend unticking "No extension" from the file filter to remove the pattern files.

Possible improvements / followups

• We could in the future expand the UI to toggle on and off certain pattern collections.

• Ability to add custom patterns (like in our Grok debugger). This can still be achieved with (?<queue_id>[0-9A-F]{10,11}) syntax.

• Point out when regex is invalid (right now it's silent).

• I've copied over the patterns from the ES repo for now, with a light script to generate an object from them. There's a CLI skeleton in place if we feel we want to actually pull these from the ES repo directly. These patterns don't change often, and aren't heavy size wise.

• Debouncing etc for processing, but I'd like to see how the final UX ends up.

Media

Screenshot

[flash1293]

This is great work - it's pretty complex to get such a thing to work reliably, but I think you made good choices to get it working in a clean way.

There is one part I'm not sure about, left a comment in the code.

Some higher level questions - you might want to tackle that on a follow-up PR but it wasn't called out explicitly so just brain dumping here:

• It seems like the current logic to do the highlighting doesn't know about which part of the highlight corresponds to which extracted value (or whether it corresponds to an extracted value at all or is just part of the pattern). What's the plan for that? At first I thought "oh that's very simple, just search for the first occurrence of the extracted value", but that might not be the right one if the same value gets extracted into multiple places. It doesn't seem like there is a really easy way to do this in a stable fashion, do you have ideas around this already?

I've copied over the patterns from the ES repo for now, with a light script to generate an object from them

• At least for the autocomplete, we probably shouldn't show all of the patterns. A lot are probably irrelevant by now (who is still using nagios?) and would be more confusing than helpful for the average users. At least we should order them in a way to put the most common ones to the top.

[flash1293]

I'm really scared of the day I need to fix a bug in here - could we add more explanatory comments to this and the parser?

Alternatively, have you looked into using a library like https://github.com/DmitrySoshnikov/regexp-tree instead of implementing it yourself? Might be a bit easier to maintain that way.

[Kerry350]

I can add more comments for sure 👍

I didn't look at using something like regexp-tree or ASTs in general, but I could look into it. I took a lot of inspiration from Grok Debugger and Grok Constructor and both just leaned on an approach like this, but I can certainly have a look at the library you've linked.

[flash1293]

Makes sense, if it's a good fit it might be worth it.

[Kerry350]

@flash1293 Thanks for reviewing.

It seems like the current logic to do the highlighting doesn't know about which part of the highlight corresponds to which extracted value (or whether it corresponds to an extracted value at all or is just part of the pattern). What's the plan for that? At first I thought "oh that's very simple, just search for the first occurrence of the extracted value", but that might not be the right one if the same value gets extracted into multiple places. It doesn't seem like there is a really easy way to do this in a stable fashion, do you have ideas around this already?

Yeah, so for the highlighting it's basically a case of "does this line match this pattern in a continuous way", and this mirrors the other Grok tools that I looked at. Values are tracked in the sense of the named fields (via capture groups and the fields property), and this becomes part of the structured output. But I haven't tracked each unique part of the highlight. Are you thinking this would be necessary for something in particular? Sorry if I've misunderstood this, this might be the part we need a call for 😁

At least for the autocomplete, we probably shouldn't show all of the patterns. A lot are probably irrelevant by now (who is still using nagios?) and would be more confusing than helpful for the average users. At least we should order them in a way to put the most common ones to the top.

Yeah, we can remove ones we don't think are needed. And I can add an order / weight to them.

[flash1293]

Are you thinking this would be necessary for something in particular

Maybe we need to chat more about the user experience with @patpscal and @LucaWintergerst - what I imagined was something where you can hover over the individual segments that are highlighted in different ways and it will tell you which part of the regex is matches (and maybe highlights both at the same time)

A bit like this on regex101.com:

[Kerry350]

@flash1293

Maybe we need to chat more about the user experience with @patpscal and @LucaWintergerst - what I imagined was something where you can hover over the individual segments that are highlighted in different ways and it will tell you which part of the regex is matches (and maybe highlights both at the same time)

Ah okay, I see what you mean.

It'll be quite hard to implement. I don't think it's impossible, but just much harder. With Regex 101 there is realistically just the one layer to deal with, regex itself. With Grok we are essentially dealing with the 3 layers of Grok > Oniguruma > JS Regex. We'd somehow need to map each part to the other parts (and it's likely why no current Grok tools support this). Like I say, possible, just hard. I've been thinking through today how the implementation would work. Monaco supports hover additions on ranges so that part, at least, shouldn't be a problem.

Do you mind if we defer that to a followup and once the UI / UX is nailed down a little more?

[LucaWintergerst]

Yes, talking to Kerry last week we also discussed that it would be much more complex now and we can follow up later.

I'll leave it up to you to decide if it's better to do the harder work now or later as I can't estimate how much extra time that is.

[flash1293]

Sure, happy to defer this - let's solve the main features first and get back to it. There's already quite some complexity in this, we don't need to frontload the hardest parts.

[flash1293]

Looks pretty good, thanks! The comments are super helpful with dense code like this.

Left two small comments about the suggestions.

[flash1293]

I'm not sure about this - when I'm at the end of the pattern so far, I guess it's more common to add a semantic name than just moving on.

By autocompleting the } this is made harder. Maybe we should even autocomplete a generic semantic name? Like if I'm typing %{WO and accept the WORD completion, it will put %{WORD:field_<n>} (with n being the number of semantic names so far).

We could even be smart about it - if you use the SPACE pattern, then you probably don't want to give a semantic name, so we autocomplete } right away.

Wdyt?

[Kerry350]

By autocompleting the } this is made harder. Maybe we should even autocomplete a generic semantic name? Like if I'm typing %{WO and accept the WORD completion, it will put %{WORD:field_} (with n being the number of semantic names so far).

Yeah, possibly. I might be missing something but I can't find a combination of these options where you can basically say "insert this, at this range, AND pull the cursor position back". So we could just omit the } entirely, but it's hard to say right now how this will be used most.

I don't agree that inserting a semantic by default is right, necessarily. Patterns without a semantic are just as valid and potentially valuable as those without. They still form a valid part of the expression, and may well be used to parse / skip parts of information but without wanting to extract them. By defaulting to adding the semantic users would have to then remove it each time they don't need, this seems more cumbersome than the opposite behaviour. (Not hard to add this one, just not sure it's better).

Not sure how the SPACE part would work accurately, as by the time you insert a space we wouldn't be offering suggestions.

[flash1293]

Should %{ be the trigger? { is just a normal character in grok, so it could trigger at the wrong time.

[Kerry350]

It's kind of hard to find detailed information on how they work, but the behaviour is the same whether this is { or %{. I think we'd probably have to do a bunch of manual inspection via the text model if this becomes a problem (nothing is actually selected even if things displaying might not be necessary).

[flash1293]

got it, not a blocker or anything.

[Kerry350]

@flash1293 Thanks for the reviews.

Just an overall note on the list here: #213278 (comment), the two main parts were implemented.

I played a bit with the automatic placeholder behaviour, I'm not 100% sure it's a good thing to add by default, I think this would make more sense with something like a toggle to turn it on / off, and will defer this for now. For displaying the colour alongside the structured output I didn't actually render this out as the UI / UX is changing anyway, but the information is 100% there to render in any way we need (cc @patpscal).

@thomheymann also had a suggestion on Slack:

I wonder if Monaco allows changing the colour for each grok pattern to match the colour you use to highlight the sample. Would make it easier to correlate the two. A bit like in the screenshot where it's changing the syntax highlighting for functions based on nesting level.

This is a good suggestion, it's just a little tricky to implement. Doing this for "proper" patterns e.g. %{WORD} is easy enough, but it becomes much harder for custom named captures e.g. (?<field_name>the pattern here). The problem is the pattern can be any amount of arbitrary regex (well, at this point it's actually Oniguruma). When we "process" the custom named patterns the only thing we actually change is swapping the name for a generated ID to track them, we don't worry about the actual custom pattern too much. Then in the samples a combination of exec() and the regex d flag (which accurately tracks index positions for capture groups) makes outputting accurate highlights pretty easy. However, doing this for our expression / grok pattern itself requires being able to 100% accurately highlight the custom named captures (including all of the abritrary Oniguruma), generating an accurate regex on our end to do this is much harder (since this can be followed by a bunch of other valid arbritrary Oniguruma too). It's the same reason the hover tooltip information is simplified for the custom capture groups.

@flash1293 Unless you see any hard blockers that can't be followed up on I'd like to get this merged in as the priorities are switching to processors 👌 (I'll remove this from the enrichment page when you're okay with it).

[flash1293]

Thanks for looking into the potential improvements, none of them are blocking, LGTM

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 33f1cd6

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
searchInferenceEndpoints	135	136	+1
streamsApp	384	385	+1
total			+2

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/grok-ui	-	2	+2
@kbn/object-utils	0	3	+3
total			+5

Unknown metric groups

API count

id	before	after	diff
@kbn/grok-ui	-	2	+2
@kbn/object-utils	8	11	+3
total			+5

ESLint disabled in files

id	before	after	diff
@kbn/grok-ui	-	1	+1

ESLint disabled line counts

id	before	after	diff
@kbn/grok-ui	-	2	+2
@kbn/object-utils	0	1	+1
total			+3

Total ESLint disabled count

id	before	after	diff
@kbn/grok-ui	-	3	+3
@kbn/object-utils	0	1	+1
total			+4

History

• 💛 Build #284484 was flaky 7bd3dea
• 💚 Build #283825 succeeded b9474c1
• 💔 Build #283290 failed 8fc4f04
• 💔 Build #281651 failed c638e77
• 💔 Build #281633 failed 767b829

cc @Kerry350

[kibanamachine]

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13935064664

[kibanamachine]

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts You might need to backport the following PRs to 8.x: - [scout] extend config-discovery with CI validator (#214403) - [Inference] Inference CLI client (#214691) - [Space time] extending Scout with perfTracker fixture (#212397) - [Streams] Replay loghub data with synthtrace (#212120) - [Unified search] Change codeowners to presentation team (#212855) - SKA: Update and breakdown x-pack/.gitignore (#212341) - Update renovate.json and remove unnecessary deps (#212214)

Manual backport

To create the backport manually run:

node scripts/backport --pr 213278

Questions ?

Please refer to the Backport tool documentation

[Kerry350]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation