Skip to content

[pkg/ottl] Fix OTTL functions by using setters #39416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
+479 −318
Open

[pkg/ottl] Fix OTTL functions by using setters #39416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c68cc6a
[pkg/ottl] Fix OTTL functions by using setters
rockdaboot Apr 15, 2025
6b38c4c
Merge branch 'main' into fix-39100
rockdaboot Apr 28, 2025
903a86d
Remove superfluous MoveTo() from flatten function
rockdaboot May 5, 2025
2ffa616
Rename targetGetter to target in merge_maps test
rockdaboot May 5, 2025
2a50ad1
Use CopyTo in test setters
rockdaboot May 5, 2025
b508ca4
Remove the 'updated' intermediate map in replace_all_matches
rockdaboot May 5, 2025
dabd3bb
Merge branch 'main' into fix-39100
rockdaboot Jun 4, 2025
1c315d5
Merge remote-tracking branch 'upstream/main' into fix-39100
rockdaboot Jun 19, 2025
8ac77ad
Tests: check that setter has been called
rockdaboot Jun 19, 2025
5437b1b
Merge remote-tracking branch 'upstream/main' into fix-39100
rockdaboot Jun 25, 2025
402aeed
Fix profiles accessAttributesKey() for e2e tests
rockdaboot Jun 25, 2025
c9e5756
Add OTTL e2e tests based on profiles
rockdaboot Jun 25, 2025
15921d0
Appease linter
rockdaboot Jun 25, 2025
30358f0
Revert last three commits in favor of #40931
rockdaboot Jun 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .chloggen/fix-ottl-functions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: pkg/ottl

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Fix OTTL functions by using setters.

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [39100]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user]
18 changes: 18 additions & 0 deletions pkg/ottl/expression.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,24 @@ func (g StandardFunctionGetter[K]) Get(args Arguments) (Expr[K], error) {
return Expr[K]{exprFunc: fn}, nil
}

type PMapGetSetter[K any] interface {
Get(ctx context.Context, tCtx K) (pcommon.Map, error)
Set(ctx context.Context, tCtx K, val pcommon.Map) error
}

type StandardPMapGetSetter[K any] struct {
Getter func(ctx context.Context, tCtx K) (pcommon.Map, error)
Setter func(ctx context.Context, tCtx K, val any) error
}

func (path StandardPMapGetSetter[K]) Get(ctx context.Context, tCtx K) (pcommon.Map, error) {
return path.Getter(ctx, tCtx)
}

func (path StandardPMapGetSetter[K]) Set(ctx context.Context, tCtx K, val pcommon.Map) error {
return path.Setter(ctx, tCtx, val)
}

// PMapGetter is a Getter that must return a pcommon.Map.
type PMapGetter[K any] interface {
// Get retrieves a pcommon.Map value.
Expand Down
10 changes: 10 additions & 0 deletions pkg/ottl/functions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,16 @@ func (p *Parser[K]) buildArg(argVal value, argType reflect.Type) (any, error) {
return nil, err
}
return StandardIntLikeGetter[K]{Getter: arg.Get}, nil
case strings.HasPrefix(name, "PMapGetSetter"):
if argVal.Literal == nil || argVal.Literal.Path == nil {
return nil, errors.New("must be a path")
}
pathGetSetter, err := p.buildGetSetterFromPath(argVal.Literal.Path)
if err != nil {
return nil, err
}
stdMapGetter := StandardPMapGetter[K]{Getter: pathGetSetter.Get}
return StandardPMapGetSetter[K]{Getter: stdMapGetter.Get, Setter: pathGetSetter.Set}, nil
case strings.HasPrefix(name, "PMapGetter"):
arg, err := p.newGetter(argVal)
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions pkg/ottl/ottlfuncs/func_delete_key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

type DeleteKeyArguments[K any] struct {
Target ottl.PMapGetter[K]
Target ottl.PMapGetSetter[K]
Key string
}

Expand All @@ -29,13 +29,13 @@ func createDeleteKeyFunction[K any](_ ottl.FunctionContext, oArgs ottl.Arguments
return deleteKey(args.Target, args.Key), nil
}

func deleteKey[K any](target ottl.PMapGetter[K], key string) ottl.ExprFunc[K] {
func deleteKey[K any](target ottl.PMapGetSetter[K], key string) ottl.ExprFunc[K] {
return func(ctx context.Context, tCtx K) (any, error) {
val, err := target.Get(ctx, tCtx)
if err != nil {
return nil, err
}
val.Remove(key)
return nil, nil
return nil, target.Set(ctx, tCtx, val)
}
}
32 changes: 23 additions & 9 deletions pkg/ottl/ottlfuncs/func_delete_key_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package ottlfuncs

import (
"context"
"errors"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -19,15 +20,22 @@ func Test_deleteKey(t *testing.T) {
input.PutInt("test2", 3)
input.PutBool("test3", true)

target := &ottl.StandardPMapGetter[pcommon.Map]{
Getter: func(_ context.Context, tCtx pcommon.Map) (any, error) {
target := &ottl.StandardPMapGetSetter[pcommon.Map]{
Getter: func(_ context.Context, tCtx pcommon.Map) (pcommon.Map, error) {
return tCtx, nil
},
Setter: func(_ context.Context, tCtx pcommon.Map, val any) error {
if v, ok := val.(pcommon.Map); ok {
v.CopyTo(tCtx)
return nil
}
return errors.New("expected pcommon.Map")
},
}

tests := []struct {
name string
target ottl.PMapGetter[pcommon.Map]
target ottl.PMapGetSetter[pcommon.Map]
key string
want func(pcommon.Map)
}{
Expand Down Expand Up @@ -80,9 +88,12 @@ func Test_deleteKey(t *testing.T) {

func Test_deleteKey_bad_input(t *testing.T) {
input := pcommon.NewValueStr("not a map")
target := &ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, tCtx any) (any, error) {
return tCtx, nil
target := &ottl.StandardPMapGetSetter[any]{
Getter: func(_ context.Context, tCtx any) (pcommon.Map, error) {
if v, ok := tCtx.(pcommon.Map); ok {
return v, nil
}
return pcommon.Map{}, errors.New("expected pcommon.Map")
},
}

Expand All @@ -94,9 +105,12 @@ func Test_deleteKey_bad_input(t *testing.T) {
}

func Test_deleteKey_get_nil(t *testing.T) {
target := &ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, tCtx any) (any, error) {
return tCtx, nil
target := &ottl.StandardPMapGetSetter[any]{
Getter: func(_ context.Context, tCtx any) (pcommon.Map, error) {
if v, ok := tCtx.(pcommon.Map); ok {
return v, nil
}
return pcommon.Map{}, errors.New("expected pcommon.Map")
},
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/ottl/ottlfuncs/func_delete_matching_keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
)

type DeleteMatchingKeysArguments[K any] struct {
Target ottl.PMapGetter[K]
Target ottl.PMapGetSetter[K]
Pattern string
}

Expand All @@ -33,7 +33,7 @@ func createDeleteMatchingKeysFunction[K any](_ ottl.FunctionContext, oArgs ottl.
return deleteMatchingKeys(args.Target, args.Pattern)
}

func deleteMatchingKeys[K any](target ottl.PMapGetter[K], pattern string) (ottl.ExprFunc[K], error) {
func deleteMatchingKeys[K any](target ottl.PMapGetSetter[K], pattern string) (ottl.ExprFunc[K], error) {
compiledPattern, err := regexp.Compile(pattern)
if err != nil {
return nil, fmt.Errorf("the regex pattern supplied to delete_matching_keys is not a valid pattern: %w", err)
Expand All @@ -46,6 +46,6 @@ func deleteMatchingKeys[K any](target ottl.PMapGetter[K], pattern string) (ottl.
val.RemoveIf(func(key string, _ pcommon.Value) bool {
return compiledPattern.MatchString(key)
})
return nil, nil
return nil, target.Set(ctx, tCtx, val)
}, nil
}
38 changes: 26 additions & 12 deletions pkg/ottl/ottlfuncs/func_delete_matching_keys_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package ottlfuncs

import (
"context"
"errors"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -20,15 +21,22 @@ func Test_deleteMatchingKeys(t *testing.T) {
input.PutInt("test2", 3)
input.PutBool("test3", true)

target := &ottl.StandardPMapGetter[pcommon.Map]{
Getter: func(_ context.Context, tCtx pcommon.Map) (any, error) {
target := &ottl.StandardPMapGetSetter[pcommon.Map]{
Getter: func(_ context.Context, tCtx pcommon.Map) (pcommon.Map, error) {
return tCtx, nil
},
Setter: func(_ context.Context, tCtx pcommon.Map, m any) error {
if v, ok := m.(pcommon.Map); ok {
v.CopyTo(tCtx)
return nil
}
return errors.New("expected pcommon.Map")
},
}

tests := []struct {
name string
target ottl.PMapGetter[pcommon.Map]
target ottl.PMapGetSetter[pcommon.Map]
pattern string
want func(pcommon.Map)
}{
Expand Down Expand Up @@ -80,9 +88,12 @@ func Test_deleteMatchingKeys(t *testing.T) {

func Test_deleteMatchingKeys_bad_input(t *testing.T) {
input := pcommon.NewValueInt(1)
target := &ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, tCtx any) (any, error) {
return tCtx, nil
target := &ottl.StandardPMapGetSetter[any]{
Getter: func(_ context.Context, tCtx any) (pcommon.Map, error) {
if v, ok := tCtx.(pcommon.Map); ok {
return v, nil
}
return pcommon.Map{}, errors.New("expected pcommon.Map")
},
}

Expand All @@ -94,9 +105,12 @@ func Test_deleteMatchingKeys_bad_input(t *testing.T) {
}

func Test_deleteMatchingKeys_get_nil(t *testing.T) {
target := &ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, tCtx any) (any, error) {
return tCtx, nil
target := &ottl.StandardPMapGetSetter[any]{
Getter: func(_ context.Context, tCtx any) (pcommon.Map, error) {
if v, ok := tCtx.(pcommon.Map); ok {
return v, nil
}
return pcommon.Map{}, errors.New("expected pcommon.Map")
},
}

Expand All @@ -107,10 +121,10 @@ func Test_deleteMatchingKeys_get_nil(t *testing.T) {
}

func Test_deleteMatchingKeys_invalid_pattern(t *testing.T) {
target := &ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, _ any) (any, error) {
target := &ottl.StandardPMapGetSetter[any]{
Getter: func(_ context.Context, _ any) (pcommon.Map, error) {
t.Errorf("nothing should be received in this scenario")
return nil, nil
return pcommon.Map{}, nil
},
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/ottl/ottlfuncs/func_flatten.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
)

type FlattenArguments[K any] struct {
Target ottl.PMapGetter[K]
Target ottl.PMapGetSetter[K]
Prefix ottl.Optional[string]
Depth ottl.Optional[int64]
ResolveConflicts ottl.Optional[bool]
Expand All @@ -43,7 +43,7 @@ func createFlattenFunction[K any](_ ottl.FunctionContext, oArgs ottl.Arguments)
return flatten(args.Target, args.Prefix, args.Depth, args.ResolveConflicts)
}

func flatten[K any](target ottl.PMapGetter[K], p ottl.Optional[string], d ottl.Optional[int64], c ottl.Optional[bool]) (ottl.ExprFunc[K], error) {
func flatten[K any](target ottl.PMapGetSetter[K], p ottl.Optional[string], d ottl.Optional[int64], c ottl.Optional[bool]) (ottl.ExprFunc[K], error) {
depth := int64(math.MaxInt64)
if !d.IsEmpty() {
depth = d.Get()
Expand Down Expand Up @@ -72,7 +72,7 @@ func flatten[K any](target ottl.PMapGetter[K], p ottl.Optional[string], d ottl.O
flattenData.flattenMap(m, prefix, 0)
flattenData.result.MoveTo(m)

return nil, nil
return nil, target.Set(ctx, tCtx, m)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to MoveTo above is already updating the map behind the scenes without needing to call target.set. Those calls, as well as any .CopyTo calls against the target map will need to be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TylerHelmuth Calling target.Set is mandatory here, since the attributes in the profiles signal can not be rendered as pcommon.Map. The setter has to take the values from the map and update two arrays, the lookup table and the array of indices into the lookup table. So for profiles, CopyTo or MoveTo without target.Set are essential no-ops and those should be removed. That's possibly what you meant, but you wrote it differently. Can you clarify, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, addressed this in func_flatten as it makes sense to me.

Copy link
Contributor

@edmocosta edmocosta May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could extend the pcommon.Map to support creating read-only shallow copies of the original Map, setting the internal state to StateReadOnly. When this Map's state is set, it ensures that no data modification is performed and panics if that happen, which IMO would be fine, considering trying to modify read-only data should be considered a programming error.

Example:

func (m Map) AsReadOnly() Map {
	state := internal.StateReadOnly
	return newMap(m.getOrig(), &state)
}

With that functionality, we could either make the PMapGetSetter Get (and probably other map getters) return read-only values, or change it on the OTTL context path getters.

func (path StandardPMapGetSetter[K]) Get(ctx context.Context, tCtx K) (pcommon.Map, error) {
	m, err := return path.Getter(ctx, tCtx)
        // ...
        return m.AsReadOnly(), nil
}

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edmocosta thats a good idea to enforce what we want.

@rockdaboot what I meant is that if we're trying to enforce that maps be set via the context's setter instead of via pdata functions, we should be consistent. In the case of flatten (and I think a majority of our editors that work on maps) we are calling ether CopyTo or MoveTo on the target instead of calling a setter. If a function does that and then also calls the .Set function, for most contexts it'll end up calling CopyTo or MoveTo twice.

Since we're being consistent calling the context's setter, we need to remove the additional CopyTo or MoveTo in the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edmocosta Good idea. Are you going to create an issue/PR? Does this PR has to wait for the new functionality or can we add read-only maps later? Just looking for clarity here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check merge_maps and the replace_all_patterns key scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edmocosta Good idea. Are you going to create an issue/PR? Does this PR has to wait for the new functionality or can we add read-only maps later? Just looking for clarity here.

I don't think we need to block this PR because of that, those changes might take some time to get through, so I'd do it later. I'll be opening the issues for implementing that soon.
The only thing I'd do in this PR is what Tyler mentioned at #39416 (comment), so when we introduce the read-only stuff it wouldn't break.

Copy link
Contributor Author

@rockdaboot rockdaboot May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check merge_maps and the replace_all_patterns key scenario.

@TylerHelmuth In both functions, the CopyTo has not pcommon.Map as source/target, instead that is on pcommon.Value. Can you give a concrete example of your idea for improvement? In ctxutil.SetMap() we could test the destination ans source map for equality and if true, skip the CopyTo() or do this check/skip inside Map.CopyTo(). But it feels out-of-scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I've opened a PR adding the read-only functions: open-telemetry/opentelemetry-collector#12995

}, nil
}

Expand Down
43 changes: 25 additions & 18 deletions pkg/ottl/ottlfuncs/func_flatten_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package ottlfuncs

import (
"context"
"errors"
"reflect"
"testing"

Expand Down Expand Up @@ -340,10 +341,19 @@ func Test_flatten(t *testing.T) {
m := pcommon.NewMap()
err := m.FromRaw(tt.target)
assert.NoError(t, err)
target := ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, _ any) (any, error) {
target := ottl.StandardPMapGetSetter[any]{
Getter: func(_ context.Context, _ any) (pcommon.Map, error) {
return m, nil
},
Setter: func(_ context.Context, _ any, m any) error {
if v, ok := m.(pcommon.Map); ok {
if dst, ok2 := m.(pcommon.Map); ok2 {
v.CopyTo(dst)
return nil
}
}
return errors.New("expected pcommon.Map")
},
}

exprFunc, err := flatten[any](target, tt.prefix, tt.depth, ottl.NewTestingOptional[bool](tt.conflict))
Expand Down Expand Up @@ -490,10 +500,19 @@ func Test_flatten_undeterministic(t *testing.T) {
m := pcommon.NewMap()
err := m.FromRaw(tt.target)
assert.NoError(t, err)
target := ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, _ any) (any, error) {
target := ottl.StandardPMapGetSetter[any]{
Getter: func(_ context.Context, _ any) (pcommon.Map, error) {
return m, nil
},
Setter: func(_ context.Context, tCtx any, m any) error {
if v, ok := m.(pcommon.Map); ok {
if dst, ok2 := tCtx.(pcommon.Map); ok2 {
v.CopyTo(dst)
}
return nil
}
return errors.New("expected pcommon.Map")
},
}

exprFunc, err := flatten[any](target, tt.prefix, tt.depth, ottl.NewTestingOptional[bool](tt.conflict))
Expand All @@ -509,18 +528,6 @@ func Test_flatten_undeterministic(t *testing.T) {
}
}

func Test_flatten_bad_target(t *testing.T) {
target := &ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, _ any) (any, error) {
return 1, nil
},
}
exprFunc, err := flatten[any](target, ottl.Optional[string]{}, ottl.Optional[int64]{}, ottl.NewTestingOptional[bool](false))
assert.NoError(t, err)
_, err = exprFunc(nil, nil)
assert.Error(t, err)
}

func Test_flatten_bad_depth(t *testing.T) {
tests := []struct {
name string
Expand All @@ -538,8 +545,8 @@ func Test_flatten_bad_depth(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
target := &ottl.StandardPMapGetter[any]{
Getter: func(_ context.Context, _ any) (any, error) {
target := &ottl.StandardPMapGetSetter[any]{
Getter: func(_ context.Context, _ any) (pcommon.Map, error) {
return pcommon.NewMap(), nil
},
}
Expand Down
Loading
Loading