[pkg/ottl] support merging multiple maps via merge_maps() function

[odubajDT]

Description

Support merging more than 2 maps via merge_maps() function

Link to tracking issue

Fixes #32954

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[odubajDT]

not stale

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[evan-bradley]

Since we're looking at breaking this, should we make the strategy optional and default to "upsert"? It would be nice if we could keep source as the second argument and just append "sources" to the end.

If we kept sources as the last argument, we could even avoid a breaking change here.

[odubajDT]

to be clear, are you suggesting:

type MergeMapsArguments[K any] struct {
	Target   ottl.PMapGetter[K]
	Source   ottl.PMapGetter[K]
	Strategy ottl.Optional[string] - default to upsert
	Sources  ottl.Optional[ottl.PMapSliceGetter[K]] - default to empty
}

Why don't then keep the Strategy as it is and the only change would be adding an optional sources parameter at the end?

Thanks!

[evan-bradley]

Why don't then keep the Strategy as it is and the only change would be adding an optional sources parameter at the end?

If we do that, then we can't make Source optional and we would need to figure out how to reconcile both Source and Sources being set.

[odubajDT]

Ok, so this is the expected state?

type MergeMapsArguments[K any] struct {
	Target   ottl.PMapGetter[K]
	Source   ottl.Optional[ottl.PMapGetter[K]]
	Strategy ottl.Optional[string] - default to upsert
	Sources  ottl.Optional[ottl.PMapSliceGetter[K]] - default to empty
}

That means, all of the parameters except the target are optional?

[evan-bradley]

Yes, that looks right. One of either Source or Sources would be required to be set though.

[odubajDT]

adapted, should be ready for a re-review

[edmocosta]

I think we can put this PR on hold for a bit while this one is not complete. If we end up merging that PR as it's right now, I think we wouldn't need to add the new type PMapSliceGetter here, and it would start supporting maps inside slices, allowing us to use arguments like this:

Sources  ottl.Optional[[]ottl.PMapGetter[K]]

[odubajDT]

I think we can put this PR on hold for a bit while this one is not complete. If we end up merging that PR as it's right now, I think we wouldn't need to add the new type PMapSliceGetter here, and it would start supporting maps inside slices, allowing us to use arguments like this:

Sources  ottl.Optional[[]ottl.PMapGetter[K]]

Can you be more specific about the solution you are proposing? In my opinion, we still need to support a SliceGetter in this case (or adapt PMapGetter), since there is no possibility that this type would accept slices.

Thanks!

[edmocosta]

Can you be more specific about the solution you are proposing? In my opinion, we still need to support a SliceGetter in this case (or adapt PMapGetter), since there is no possibility that this type would accept slices.

The ottl.PMapGetter[K] was changed through that PR (see), and OTTL now supports slices of ottl.PMapGetter[K] parameters, like it does for other types.
It means we probably don't need a new type for that anymore (StandardPMapSliceGetter), and instead, we can just change the Sources type to ottl.Optional[[]ottl.PMapGetter[K]], and that should work as expected.

[odubajDT]

Can you be more specific about the solution you are proposing? In my opinion, we still need to support a SliceGetter in this case (or adapt PMapGetter), since there is no possibility that this type would accept slices.

The ottl.PMapGetter[K] was changed through that PR (see), and OTTL now supports slices of ottl.PMapGetter[K] parameters, like it does for other types. It means we probably don't need a new type for that anymore (StandardPMapSliceGetter), and instead, we can just change the Sources type to ottl.Optional[[]ottl.PMapGetter[K]], and that should work as expected.

The PR you linked does not adapt PMapGetter but mapGetter which covers the part of literals parsing. These things do not have nothing in common here.

You actually need to introduce a functionality that handles the extraction of slice of maps into []pcommon.Map, therefore introduce a separate PMapSliceGetter. The reasoning is that whenever you call Get(context, context) on a single item of []PMapGetter, the context will always return a value of type []pcommon.Map and therefore ends up with an error.

This part is not handled in PMapGetter.

[edmocosta]

I'm sorry, but I'm still failing to understand why we would need a specific type for that, since OTTL now supports maps inside slices. The only thing I think it would change is the way we access the data, for example, using the suggested slice of maps, it would looks like this:

type MergeMapsArguments[K any] struct {
   ...
  Sources  ottl.Optional[[]ottl.PMapGetter[K]]
}

func mergeMaps[K any](target ottl.PMapGetter[K], source ottl.Optional[ottl.PMapGetter[K]], strategy ottl.Optional[string], sources ottl.Optional[[]ottl.PMapGetter[K]]) (ottl.ExprFunc[K], error) {
                 ....
		if !sources.IsEmpty() {
			sourcesMapsSlice := sources.Get()
			for _, sourceGetter := range sourcesMapsSlice {
				val, err := sourceGetter.Get(ctx, tCtx)
				if err != nil {
					return nil, err
				}
				if err := merge(mergeStrategy, &val, &targetMap); err != nil {
					return nil, err
				}
			}
		}
                ....
}

That should support not only literal values, but any map getter, including paths:

log_statements:
 - context: log
   statements:
    - 'merge_maps(attributes, sources = [{"foo": "bar"}, resource.attributes])'

Am I missing something?

[odubajDT]

I'm sorry, but I'm still failing to understand why we would need a specific type for that, since OTTL now supports maps inside slices. The only thing I think it would change is the way we access the data, for example, using the suggested slice of maps, it would looks like this:

type MergeMapsArguments[K any] struct {
   ...
  Sources  ottl.Optional[[]ottl.PMapGetter[K]]
}

func mergeMaps[K any](target ottl.PMapGetter[K], source ottl.Optional[ottl.PMapGetter[K]], strategy ottl.Optional[string], sources ottl.Optional[[]ottl.PMapGetter[K]]) (ottl.ExprFunc[K], error) {
                 ....
		if !sources.IsEmpty() {
			sourcesMapsSlice := sources.Get()
			for _, sourceGetter := range sourcesMapsSlice {
				val, err := sourceGetter.Get(ctx, tCtx)
				if err != nil {
					return nil, err
				}
				if err := merge(mergeStrategy, &val, &targetMap); err != nil {
					return nil, err
				}
			}
		}
                ....
}

That should support not only literal values, but any map getter, including paths:

log_statements:
 - context: log
   statements:
    - 'merge_maps(attributes, sources = [{"foo": "bar"}, resource.attributes])'

Am I missing something?

I made exactly these changes previously and it won't work due to reasons I mentioned. Did you tried out the code snipped you sent me? If not, please do so. I might be missing something, but this solution won't work in my opinion

[odubajDT]

Oh I see now, thanks! It seems the issue is when we pass a map key path argument, which underline type is a slice of maps. In that case, it indeed won't work.

log_statements:
 - context: log
   statements:
    - 'set(attributes["slice"], [{"foo": "bar"}])'
    - 'merge_maps(attributes, sources = attributes["slice"])'

It's affecting other types and functions using slices as well, (e.g Concat), but I'm not pretty sure if that can be easily solved in a generic way, without having to create an custom slice type for every value type.
Do you have any thoughts on that @TylerHelmuth @evan-bradley? Is it a known OTTL limitation or bug? What do you think we should do here? go ahead with the slices and later tackle this issue globally, or use the custom type for now?

Thanks for checking it!

I guess in this case we should go with a custom slice type. Opened for other opinions

ping @edmocosta @TylerHelmuth @evan-bradley

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.