INGEST: Extend KV Processor (#31789)

[original-brownbear]

Added more capabilities supported by LS to the KV processor:

• Stripping of brackets and quotes from values (include_brackets in corresponding LS filter)
• Adding key prefixes
• Trimming specified chars from keys and values

Refactored the way the filter is configured to avoid conditionals during execution.
Refactored Tests a little to not have to add more redundant getters for new parameters.

relates to #31786

[elasticmachine]

Pinging @elastic/es-core-infra

[original-brownbear]

@rjernst @talevy this should be good to review.
Added all options that I figured would be relevant to help with problems like #31786 that LS has but Ingest doesn't.
Left out some other options, that are kind of pointless in Ingest (like stripping arbitrary chars from any position in keys and values) and can better be handled by grok or gsub.

[jakelandis]

@original-brownbear I think all the changes here are good ... but I don't think it resolves #31786

If I understand correctly the #31786 is to treat anything in quotes for the value as a single entity. For example:

POST _ingest/pipeline/_simulate
{
  "pipeline" : {
    "processors" : [
      { "kv": 
        { "field": "message",
          "field_split": " ",
          "value_split": "="
        }
      }
    ]
  },
  "docs" : [
     {"_source": {
       "message": "a=foo b='bar baz' c=lol"
     }}
    ]
}

should result in b=bar baz , but since " " (space) is the split value, the code doesn't take into account that bar baz is inside a quote and results in an error does not contain value_split [=]. You can also reproduce this with any other field_split such as "field_split": "," with "message": "a=foo,b='bar,baz',c=lol"

[original-brownbear]

@jakelandis yea, the example you're making is still not fixed, but that won't work in LS either (unless you're doing a bunch of Regex trickery).
But at least with this change we get some parity with LS for the case of e.g. all args quoted because we can neatly strip the delimiters now:

e.g.

POST _ingest/pipeline/_simulate
{
  "pipeline" : {
    "processors" : [
      { "kv": 
        { "field": "message",
          "field_split": "' ",
          "strip_brackets": true,
          "value_split": "="
        }
      }
    ]
  },
  "docs" : [
     {"_source": {
       "message": "a='foo' b='bar baz' c='lol'"
     }}
    ]
}

{
  "docs": [
    {
      "doc": {
        "_index": "_index",
        "_type": "_type",
        "_id": "_id",
        "_source": {
          "a": "foo",
          "b": "bar baz",
          "c": "lol",
          "message": "a='foo' b='bar baz' c='lol'"
        },
        "_ingest": {
          "timestamp": "2018-07-20T15:55:56.635799Z"
        }
      }
    }
  ]
}

I think that's what this comment #31786 (comment) is basically asking for after we suggest that the tricky mix of quotes and no quotes is better handled by Grok.

[jakelandis]

did you want to call out these are regex character classes ?

edit: clarified question

[original-brownbear]

@jakelandis I'd rather not talk about regex patterns here I think. If I do that instead of simply saying I'm trimming all the chars in the given string, then that's probably not really true if you look at the code (I'm building a pattern from the string, arbitrary patterns won't work here).

[jakelandis]

@original-brownbear - fair points.

I think we could change the split on the field to a sequence of grouped regex pattern matches to enable support for mixed quotes. I took a quick swing but came up a bit short, and unfortunately you can't stream pattern matches (out of the box) till java9 (which would be useful for your functional pattern used here).

I did some light testing and the code looks good. So LGTM, but may want to wait for one more given my experience on this code base.

Also, can you leave #31786 open ? This change helps, but does not solve the original intent of that issue, and I think (given a bit more time) that a solution to the original issue is feasible.

[original-brownbear]

@jakelandis thanks, sure I can leave it open, would be neat if we found an elegant solution here that isn't just a tricky regex :)

[original-brownbear]

@rjernst could you take a look here? :)

[rjernst]

I defer to @talevy on this one, as he is much more familiar with the logstash side this PR intends to mimic.

[talevy]

Cool! I think it is great to go through the shortcomings of some of our existing processors.

This comment (#32232 (comment)) causes me to wonder a few things. If this isn't fixing the issue, why are we claiming it closes the issue?

I have been away from KV use-cases in Logstash for a while, but I remember its specific behavior was built for very specific application log-lines in mind. Documenting this would be helpful. The reason it lacked parity in the first place was because the case for doing this extra regex magic did not feel right in a processor that simply is called "Key-Value". It felt like it was doing a lot more. I think it is totally reasonable to re-evaluate this and bring in the parity, but I think it would be helpful to bring in more examples as to why it does all this. If it is doing these extra things because a handful of specific log lines do these things, it may be best to provide examples of how to use grok+kv together to achieve proper parsing, rather than to bake in heuristics into this processor.

Let me know your thoughts!

[original-brownbear]

@talevy I think I would agree with @jakelandis here that there should be an easy and clean way to achieve this kind of parsing in KV.
My gut feeling is, that it would be ok to simply handle <([ and '" as special cases here and enable their use as delimiters for matching values somehow (before the actual splitting happens). It seems from looking at the KV filter that the stripping of these delimiters is pretty standard (in fact it's LS default behaviour, here in Ingest I had to make it the optional behaviour to not change existing behaviour) => may be ok to simply identify delimited groups first, then split pairs and then extract the KVs? (just a thought for how to achieve this, I'm also fine with simply deferring to Grok here).

Probably better to have this discussion in the related issue though :), this PR is really more about getting feature parity with LS. I changed the PR description accordingly to "relates" instead of "fixes" :)

[talevy]

thanks @original-brownbear. that helps. I think that is a good plan

[original-brownbear]

@talevy sweet :) Would you still review this one as a step 1 towards that?

[talevy]

yup! @original-brownbear

[talevy]

Overall looks good!

In addition to successful tests of trimming keys/values, and strip_brackets. Should we have tests for those when set and do not match the input keys/values? So, when their patterns do not match

[talevy]

should this be renamed to be more general than just brackets. maybe strip_surrounding

[original-brownbear]

@talevy yea I agree, brackets isn't a good term here, I went with it because that's what LS uses (see https://github.com/logstash-plugins/logstash-filter-kv/blob/master/lib/logstash/filters/kv.rb#L290). surrounding sounds kinda strange too, not that I have a better suggestion ... maybe strip_delimiters but then that's somewhat weird too since we're already splitting twice elsewhere :P

[talevy]

that is part of the hidden magic that I didn't love in that filter. But I agree, I cannot think of a better word, and having a separate property just for quotes feels like too much variation

[talevy]

I think that we should have additional explicit tests of the factory as we do in KeyValueProcessorFactoryTests for these new config options.

[original-brownbear]

@talevy but that means adding more getters and fields to the processor that will just be test only code? Testing external behaviour here seems fine. @rjernst wasn't so happy about that approach in another PR and I tend to agree tbh.

[talevy]

OK

[original-brownbear]

@talevy when patterns don't match nothing happens, I think that's kinda redundant to test isn't it? (Especially since this already happens implicitly in those tests that mix a few quote/bracket kinds)

[talevy]

@original-brownbear true. I guess I was looking for something more explicit just in case there was different behavior when none match.

[talevy]

LGTM

[original-brownbear]

@talevy thanks!

[jakelandis]

I have POC [1] for supporting quotes (lifted some code from my dissect implementation). Reg-ex's got real ugly real quick. I will work on getting it cleaned up and better tested and submit a PR.

[1]
jakelandis@0653ae1