[pkg/ottl] Add merge_maps function

[TylerHelmuth]

Description:
Adds a new function that is able to gracefully merge a source map into a target map. If target already contains a key from source, the source's value is used.

For efficiency, I opted for a Function for this capability instead of a Factory Function combined with set for efficiency.

Link to tracking Issue:
Related to #9410
Related to #14938

Testing:
added unit tests

Documentation:
updated function doc

[evan-bradley]

I agree that making this a function instead of a factory function is the right move in terms of efficiency for probably the majority of use cases. However, I can also see more complex uses arising for transforming maps, like KeepKeys(MergeMaps(ParseJSON(body), resource.attributes), "http_.*"). One way to do both could be to have factory function versions of some functions, like merge_maps and MergeMaps, but that risks being confusing to users. Alternatively, we could sacrifice some efficiency and standardize on using factory functions with set, or just forgo the factory function versions for now until there are requests for more complex functionality. What do you think?

[evan-bradley]

Not necessarily an issue that needs to be solved in this PR, but I think we should do something to notify the user that they've provided an invalid value. Should we emit a debug log if either of the values aren't maps?

[TylerHelmuth]

Ya this is a problem with lots of our functions. Need to figure out a good way to guarantee access to the telemetrysettings.Logger in all functions. We could enforce telemetrysettings to always be the first param as a start.

[bogdandrutu]

Why not just "merge" and say for the moment only works with maps?

[TylerHelmuth]

I can try this out. Is slice the only other type that we'd support in a merge function?

[TylerHelmuth]

@bogdandrutu updated the name to merge, but with that addition of the strategy param, is there another type that you think we'll be supporting in a merge function in the future?

[TylerHelmuth]

@evan-bradley I think in the situation of KeepKeys(MergeMaps(ParseJSON(body), resource.attributes), "http_.*"), to take advantage of the result of keep keys you still have to then call set, so the final result is

set(resource.attributes, KeepKeys(MergeMaps(ParseJSON(body), resource.attributes), "http_.*"))

If I am counting correctly thats 1 loop inside MergeMaps and a new map created, 1 loop inside KeepKeys and a new map created, and then 1 loop inside set to copy the new value. Since Factory Functions are pure (since they don't know the source of the input) both KeepKeys and MergeMaps have to create new maps and can't modify their inputs.

If we go with merge_map then the same result, spread over 2 statements, looks like

merge_maps(resource.attributes, ParseJSON(body))
set(resource.attributes, KeepKeys(resource.attributes, "http_.*"))

I think that results in 1 loop within merge_maps, without having to create a new map, 1 loop instead KeepKeys and a new map created, and then 1 loop inside set to copy the new value. We may also be able to hold off on a function like KeepKeys if delete_matching_keys can suffice. So I think this ends but with a pretty good performance gain. Downside is that it doesn't let you keep resource.attributes as the default values so I think merge_maps will need an extra param that specifies who wins in a tie.

Overall going the Factory Function route would mean more flexibility, but I kinda like the simplicity of merge_maps and its performance gains. What do you think?

[TylerHelmuth]

@djaglowski @evan-bradley @bogdandrutu @kentquirk ready for re-review.

[evan-bradley]

Overall going the Factory Function route would mean more flexibility, but I kinda like the simplicity of merge_maps and its performance gains.

I definitely agree here, just wanted to bring up a few options. I think this implementation is the best route for now, we can consider additional paths forward if we need more flexibility.

[TylerHelmuth]

@evan-bradley thanks for the great discussion. I think the more log transformations we enable the more times we're gonna have to decide whether we want to chain factory functions or if multiple statements can handle the use case.

Once conditions get involved its nice to be able to do things in a single statement so that the condition doesn't have to be spread between multiple statements. Could be a reason to allow OTTL's grammar to support multiple Invocations per condition.

[evan-bradley]

Looks good to me. I think we should probably keep the name as merge_maps considering the merge strategies don't make sense for slices, and having disjoint sets of strategies in a single function signature seems like a likely source of confusion.

[djaglowski]

I like the strategy parameter and associated values. An additional aspect of merging to consider is recursion. Should we have a way to support the following?

target:
  a: 1
  b: 2
  details:
    important: foo

source:
  b: 3
  c: 4
  details:
    not_important: bar

result:
  a: 1
  b: 3
  c: 4
  details:
    important: foo
    not_important: bar  

[TylerHelmuth]

@djaglowski that's a good point. How does Stanza handle this?

[djaglowski]

It doesn't actually, just works like upsert at the immediate level only.

I don't feel strongly that we should support it. It introduces a lot of complexity and it's not clearly needed.

[TylerHelmuth]

@djaglowski In that case I'd vote for moving this forward as is for now and supporting that capability once we have a specific request/need.