enhancement: created new lookup library

[StephenWakely]

Ref #7072

A rough first draft that creates a new lookup library, just to check how hard it would be. Vrl integrates with this and parses it's paths as LookupBuf so they can be used directly with the Lookup Api.

This compiles, but likely doesn't work properly. Since Vrl also does it's own lookups against it's internal values, that code also needs changing - I don't think I have it correct at present.

This doesn't deal with coalescing yet. Currently lookup allows nested coalescing - (stuff like .(a | (b | c) | d).e), whereas Vrl does not. I would suggest it isn't necessary and we should take the simpler option, since it has been questioned if coalescing is needed at all outside Vrl.

This is just using the owned version of Lookups. I think that should be ok for Vrl since it should only need to create lookups at boot time, but that will need looking into.

The next step would be to fix the integration between Vrl and Vector, but that looks like it should be a pretty simple operation now that Vrl is using LookupBufs. It should also be possible to do that without affecting the rest of Vector.

Signed-off-by: Stephen Wakely fungus.humungus@gmail.com

[binarylogic]

since it has been questioned if coalescing is needed at all outside Vrl.

That is correct. We do not need path coalescing outside of VRL.

[StephenWakely]

This is now ready for review.

There are two lookup types - Lookup<'a> - the referenced type and LookupBuf - the owned type.

There is a new library lib/lookup that handles the lookup interface. This library contains the core lookup types as well and handles the parsing (using lalrpop). Vrl still parses paths itself - but also creates LookupBuf objects. There are thus two places in Vector that parse paths. I wasn't able to work out a good way to share the code, but figure it is a fairly simple function anyway. There are slight differences - lookup parses into Lookup<'a> so the result shares the same lifetime as the input string, vrl parses into LookupBuf - the owned variant. Since Vrl parses these paths at boot time, the extra allocations shouldn't have too big an impact.

The actual lookup occurs in Value. A Value can essentially take a Lookup to add, insert and remove data contained within it. add and remove takes a Lookup<'a>, whereas inserttakes a LookupBuf. I haven't yet investigated why there is a difference here and would like to try to make insert take a Lookup if possible.

LogEvent has been changed so instead of holding a BTreeMap<String, Value> to represent the log fields it now holds a Value. This Value has to be a Value::Map. If it doesn't Vector will likely panic very quickly. I am a little bit uneasy about this - although I do think it was me that originally suggested we do it this way. It does mean that all the lookup code can hang on Value and doesn't need any special code to handle the initial lookup in LogEvent, but I don't like the fact that a potential edge case could turn into a runtime error.

Coalescing is still possible - both in Vrl and Vector, but I have made it single level only. You can only coalesce a single level of fields. Before you could coalesce arbitrarily nested levels of lookups. This is no longer possible: .(thing | thang.(thong | thung[0]).(thing.thung.therng | thingy)). This has drastically simplified some of the lookup code!

With this PR it will only be Vrl that uses the Lookup code. It will be interesting to profile this to see if it has boosted performance on a Vrl rich config. The rest of Vector will still use the original lookup code and can be moved over in stages as and when!

I should add that the credit for the vast majority of this work goes to @Hoverbear.

[blt]

I have some comments that could use some attention but overall I'm very for this change set and they are not blocking. Nice work.

[JeanMertz]

I left a few remarks, but overall this looks good. Nice job!

I probably would have preferred if we had introduced the lookup crate in a separate PR, instead of also updating VRL in the same PR. I get that one is needed to drive the API of the other, but that can still be done locally, without combining them into one PR.

Just something to keep in mind for the future, not relevant any more.

Also, I found one or two lookup public API methods that we aren't using. I wonder how many more there are that seem "nice to have", but aren't actually in use. I'd prefer it if we kept the library as small as possible to only provide the functionality we need at this moment, we can always extend it in the future if need be. Right now, it's just dead code that we have to maintain/test for no apparent gain.

[JeanMertz]

Do we want to add a test to validate this?

[StephenWakely]

From what I can tell, I haven't changed the actual result of the code here. But I'm not really sure what it is doing? Why is it taking the last field of the coalesce and returning an Object typekind with that fieldname? Do you have any ideas?

[bruceg]

Could the clone be avoided with a Lookup<'a>? BTW you should be able to take multiple immutable borrows.

[StephenWakely]

I don't think it's any better:

        for sub_segment in sub_segments {
            let segment: Segment = sub_segment.name.as_str().into();
            let mut lookup = Lookup::from(segment);
            let new_working_lookup: Lookup = (&working_lookup).into();
            lookup.extend(new_working_lookup); // We need to include the rest of the insert.

            if !sub_value.contains(lookup) {
                let mut lookup: LookupBuf = LookupBuf::from(sub_segment);
                lookup.extend(working_lookup);
                needle = Some(lookup);
                break;
            }
        }

There's still a number of allocations, it might be slightly better than the clone, but that's questionable. I'm sure it could be improved further and it's probably worth keeping in mind for future work, but for now I think we should leave it as it is..

[bruceg]

Any thoughts on (another) lookup reference type to help avoid some clones, possibly for future work? This might allow for working better with AsRef or Deref.

enum LookupRef<'a> {
    Lookup(Lookup<'a>), // (or maybe needs to be &'a Lookup<'a> ?)
    LookupBuf(&'a LookupBuf),
}

Just thinking aloud, mostly.

[StephenWakely]

I am getting the following perf numbers with these confs:

2b150a3 is master,
67fd681 is this branch.

 _                            __ _
| |__  _   _ _ __   ___ _ __ / _(_)_ __   ___
| '_ \| | | | '_ \ / _ \ '__| |_| | '_ \ / _ \
| | | | |_| | |_) |  __/ |  |  _| | | | |  __/
|_| |_|\__, | .__/ \___|_|  |_| |_|_| |_|\___|
       |___/|_|
PROCESSING: remap-multihop.toml
Benchmark #1: zcat baked.log.gz | /mnt/sda2/src/perf/bin/vector-2b150a3e4feea493dde4cd85925458812208fd73 -qq --config /mnt/sda2/src/perf/configs/remap-multihop.toml
  Time (mean ± σ):     24.615 s ±  0.147 s    [User: 101.098 s, System: 6.210 s]
  Range (min … max):   24.363 s … 24.868 s    10 runs

Benchmark #2: zcat baked.log.gz | /mnt/sda2/src/perf/bin/vector-67fd681159d13bf61512206ccf2c87cbce51c487 -qq --config /mnt/sda2/src/perf/configs/remap-multihop.toml
  Time (mean ± σ):     21.868 s ±  0.268 s    [User: 87.959 s, System: 3.900 s]
  Range (min … max):   21.608 s … 22.359 s    10 runs

Summary
  'zcat baked.log.gz | /mnt/sda2/src/perf/bin/vector-67fd681159d13bf61512206ccf2c87cbce51c487 -qq --config /mnt/sda2/src/perf/configs/remap-multihop.toml' ran
    1.13 ± 0.02 times faster than 'zcat baked.log.gz | /mnt/sda2/src/perf/bin/vector-2b150a3e4feea493dde4cd85925458812208fd73 -qq --config /mnt/sda2/src/perf/configs/remap-multihop.toml'
PROCESSING: remap-nohop.toml
Benchmark #1: zcat baked.log.gz | /mnt/sda2/src/perf/bin/vector-2b150a3e4feea493dde4cd85925458812208fd73 -qq --config /mnt/sda2/src/perf/configs/remap-nohop.toml
  Time (mean ± σ):     45.134 s ±  1.260 s    [User: 105.014 s, System: 13.426 s]
  Range (min … max):   43.411 s … 46.618 s    10 runs

Benchmark #2: zcat baked.log.gz | /mnt/sda2/src/perf/bin/vector-67fd681159d13bf61512206ccf2c87cbce51c487 -qq --config /mnt/sda2/src/perf/configs/remap-nohop.toml
  Time (mean ± σ):     34.401 s ±  1.024 s    [User: 84.914 s, System: 9.520 s]
  Range (min … max):   33.587 s … 36.130 s    10 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Summary
  'zcat baked.log.gz | /mnt/sda2/src/perf/bin/vector-67fd681159d13bf61512206ccf2c87cbce51c487 -qq --config /mnt/sda2/src/perf/configs/remap-nohop.toml' ran
    1.31 ± 0.05 times faster than 'zcat baked.log.gz | /mnt/sda2/src/perf/bin/vector-2b150a3e4feea493dde4cd85925458812208fd73 -qq --config /mnt/sda2/src/perf/configs/remap-nohop.toml'
     

If I'm reading the numbers right (@blt can you confirm), the multihop config is 13% faster and the no hop, all in on VRL config, is 31% faster.

[blt]

@StephenWakely you're reading that right and I can reproduce similar numbers on my system. Nice work.

[bruceg]

Any thoughts on (another) lookup reference type to help avoid some clones, possibly for future work? This might allow for working better with AsRef or Deref.

enum LookupRef<'a> {
    Lookup(Lookup<'a>), // (or maybe needs to be &'a Lookup<'a> ?)
    LookupBuf(&'a LookupBuf),
}

Just thinking aloud, mostly.