Skip to content
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
5e16669
adapt return types of mapGetter to raw, and ValueExpression getter to…
bacherfl Jan 20, 2025
af876fe
return raw types in mapGetter
bacherfl Jan 22, 2025
4d8ab84
add e2e test and changelog entry
bacherfl Jan 22, 2025
cd28214
Merge branch 'main' into fix/ottl/map-types
bacherfl Jan 22, 2025
d21702e
fix linting
bacherfl Jan 22, 2025
f92e743
Merge remote-tracking branch 'bacherfl/fix/ottl/map-types' into fix/o…
bacherfl Jan 22, 2025
674887f
Merge branch 'main' into fix/ottl/map-types
bacherfl Jan 22, 2025
659705e
keep returning pcommon.Map in mapGetter, handle edge case in listGett…
bacherfl Jan 23, 2025
09518c1
fix linting
bacherfl Jan 23, 2025
86ebd2b
Merge branch 'main' into fix/ottl/map-types
bacherfl Jan 23, 2025
a376ab4
apply suggestion from code review
bacherfl Jan 23, 2025
38a56d6
fix linting
bacherfl Jan 23, 2025
1ca8c3c
Merge branch 'main' into fix/ottl/map-types
bacherfl Jan 29, 2025
e8528af
Merge branch 'main' into fix/ottl/map-types
bacherfl Feb 3, 2025
65dddf6
Merge branch 'main' into fix/ottl/map-types
bacherfl Feb 3, 2025
9cb30b1
Merge branch 'main' into fix/ottl/map-types
bacherfl Feb 4, 2025
c97ca85
Merge branch 'main' into fix/ottl/map-types
bacherfl Feb 6, 2025
755bb08
add further test cases
bacherfl Feb 12, 2025
18780d3
handle case where list contains maps in ValueExpression
bacherfl Feb 13, 2025
371185c
fix linting
bacherfl Feb 13, 2025
fd07e48
Merge branch 'main' into fix/ottl/map-types
bacherfl Feb 13, 2025
3631f49
additional test cases
bacherfl Feb 13, 2025
cde18ff
Merge remote-tracking branch 'bacherfl/fix/ottl/map-types' into fix/o…
bacherfl Feb 13, 2025
820efc7
fix linting
bacherfl Feb 13, 2025
8266862
Merge branch 'main' into fix/ottl/map-types
bacherfl Feb 24, 2025
f310cf5
Merge branch 'main' into fix/ottl/map-types
bacherfl Feb 26, 2025
b3778fb
Merge branch 'main' into fix/ottl/map-types
bacherfl Mar 3, 2025
22ee599
Merge branch 'main' into fix/ottl/map-types
bacherfl Mar 4, 2025
6b6711a
Merge branch 'main' into fix/ottl/map-types
bacherfl Mar 4, 2025
b4065f8
Merge branch 'main' into fix/ottl/map-types
bacherfl Mar 4, 2025
f82503e
add more tests
bacherfl Mar 6, 2025
676d591
add test cases for multiple statement executions
bacherfl Mar 6, 2025
525dd97
Merge branch 'main' into fix/ottl/map-types
bacherfl Mar 6, 2025
c300c29
fix linting
bacherfl Mar 6, 2025
e98bfba
Merge branch 'main' into fix/ottl/map-types
bacherfl Mar 10, 2025
9c343b0
return pcommon types in value expressions
bacherfl Mar 10, 2025
015e84b
fix linting
bacherfl Mar 10, 2025
8227837
return []any instead of pcommon.Slice
bacherfl Mar 11, 2025
35e21a8
Merge branch 'main' into fix/ottl/map-types
bacherfl Mar 11, 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/ottl-nested-map-literals.yaml
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 limitation of map literals within slice literals not being handled correctly

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

# (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: []
46 changes: 46 additions & 0 deletions pkg/ottl/e2e/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,30 @@ func Test_e2e_converters(t *testing.T) {
m.PutInt("bar", 5)
},
},
{
statement: `set(attributes["test"], {"list":[{"foo":"bar"}]})`,
want: func(tCtx ottllog.TransformContext) {
m := tCtx.GetLogRecord().Attributes().PutEmptyMap("test")
m2 := m.PutEmptySlice("list").AppendEmpty().SetEmptyMap()
m2.PutStr("foo", "bar")
},
},
{
statement: `set(attributes, {"list":[{"foo":"bar"}]})`,
want: func(tCtx ottllog.TransformContext) {
tCtx.GetLogRecord().Attributes().Clear()
m2 := tCtx.GetLogRecord().Attributes().PutEmptySlice("list").AppendEmpty().SetEmptyMap()
m2.PutStr("foo", "bar")
},
},
{
statement: `set(attributes["arr"], [{"list":[{"foo":"bar"}]}, {"bar":"baz"}])`,
want: func(tCtx ottllog.TransformContext) {
arr := tCtx.GetLogRecord().Attributes().PutEmptySlice("arr")
arr.AppendEmpty().SetEmptyMap().PutEmptySlice("list").AppendEmpty().SetEmptyMap().PutStr("foo", "bar")
arr.AppendEmpty().SetEmptyMap().PutStr("bar", "baz")
},
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -1263,6 +1287,28 @@ func Test_e2e_ottl_value_expressions(t *testing.T) {
statement: `Hex(Len(attributes) + Len(attributes))`,
want: "0000000000000018",
},
{
name: "return map type",
statement: `attributes["foo"]`,
want: map[string]any{
"bar": "pass",
"flags": "pass",
"slice": []any{
"val",
},
"nested": map[string]any{
"test": "pass",
},
},
},
{
name: "return list",
statement: `attributes["things"]`,
want: []any{
map[string]any{"name": "foo", "value": int64(2)},
map[string]any{"name": "bar", "value": int64(5)},
},
},
}

for _, tt := range tests {
Expand Down
31 changes: 23 additions & 8 deletions pkg/ottl/expression.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ func (l *listGetter[K]) Get(ctx context.Context, tCtx K) (any, error) {
if err != nil {
return nil, err
}

evaluated[i] = val
}

Expand All @@ -180,23 +181,37 @@ type mapGetter[K any] struct {
}

func (m *mapGetter[K]) Get(ctx context.Context, tCtx K) (any, error) {
evaluated := map[string]any{}
result := pcommon.NewMap()
for k, v := range m.mapValues {
val, err := v.Get(ctx, tCtx)
if err != nil {
return nil, err
}
switch t := val.(type) {
switch typedVal := val.(type) {
case pcommon.Map:
evaluated[k] = t.AsRaw()
target := result.PutEmpty(k).SetEmptyMap()
typedVal.CopyTo(target)
case []any:
target := result.PutEmpty(k).SetEmptySlice()
for _, el := range typedVal {
switch typedEl := el.(type) {
case pcommon.Map:
m := target.AppendEmpty().SetEmptyMap()
typedEl.CopyTo(m)
default:
err := target.AppendEmpty().FromRaw(el)
if err != nil {
return nil, err
}
}
}
default:
evaluated[k] = t
err := result.PutEmpty(k).FromRaw(val)
if err != nil {
return nil, err
}
}
}
result := pcommon.NewMap()
if err := result.FromRaw(evaluated); err != nil {
return nil, err
}
return result, nil
}

Expand Down
12 changes: 11 additions & 1 deletion pkg/ottl/expression_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,16 @@ func Test_newGetter(t *testing.T) {
Int: ottltest.Intp(1),
},
},
{
Map: &mapValue{
Values: []mapItem{
{
Key: ottltest.Strp("stringAttr"),
Value: &value{String: ottltest.Strp("value")},
},
},
},
},
},
},
},
Expand All @@ -660,7 +670,7 @@ func Test_newGetter(t *testing.T) {
"foo": map[string]any{
"test": "value",
},
"listAttr": []any{"test0", int64(1)},
"listAttr": []any{"test0", int64(1), map[string]any{"stringAttr": "value"}},
},
"stringAttr": "value",
"intAttr": int64(3),
Expand Down
26 changes: 25 additions & 1 deletion pkg/ottl/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/alecthomas/participle/v2"
"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/pdata/pcommon"
"go.uber.org/zap"
)

Expand Down Expand Up @@ -480,6 +481,29 @@ func (p *Parser[K]) ParseValueExpression(raw string) (*ValueExpression[K], error
}

return &ValueExpression[K]{
getter: getter,
getter: &StandardGetSetter[K]{
Copy link
Contributor

Choose a reason for hiding this comment

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

If we end up stick to this approach, considering the mapGetter is now returning pcommon.Map values, I think we could keep it consistent and remove these changes, so all maps would still be parsed aspcommon.Map instead of raw values. I guess it would be a breaking change, though. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be fine with that, however there are also arguments for returning the raw types here (#37280 (comment)). I think I would also prefer to consistently return pcommon.Map values though. @evan-bradley @TylerHelmuth WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Judging by #37280 (comment), we probably need to survey this a bit more to make sure we're either consistent everywhere, or can translate at any given point in the chain. I'm not sure I have a strong opinion here, but it sounds like at a minimum we could potentially use more tests.

Do the same issues apply to slices with []any vs. pcommon.Slice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for slices, this issue doesn't apply, as the listGetter returns []any as opposed to pcommon.Slice:

func (l *listGetter[K]) Get(ctx context.Context, tCtx K) (any, error) {
evaluated := make([]any, len(l.slice))
for i, v := range l.slice {
val, err := v.Get(ctx, tCtx)
if err != nil {
return nil, err
}
evaluated[i] = val
}
return evaluated, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @edmocosta that we should continue returning pcommon.Map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I now changed it again to return pcommon.Map

Getter: func(ctx context.Context, tCtx K) (any, error) {
val, err := getter.Get(ctx, tCtx)
if err != nil {
return nil, err
}
switch v := val.(type) {
case pcommon.Map:
return v.AsRaw(), nil
case pcommon.Slice:
return v.AsRaw(), nil
case []any:
for index, elem := range v {
// make sure also nested maps within a slice are returned in their raw form
if m, ok := elem.(pcommon.Map); ok {
v[index] = m.AsRaw()
}
}
return v, nil
default:
return v, nil
}
},
},
}, nil
}
27 changes: 21 additions & 6 deletions pkg/ottl/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/collector/component/componenttest"
"go.opentelemetry.io/collector/pdata/pcommon"

"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest"
)
Expand Down Expand Up @@ -2209,11 +2208,9 @@ func Test_parseValueExpression_full(t *testing.T) {
name: "map",
valueExpression: `{"map": 1}`,
expected: func() any {
m := pcommon.NewMap()
_ = m.FromRaw(map[string]any{
"map": 1,
})
return m
return map[string]any{
"map": int64(1),
}
},
},
{
Expand All @@ -2223,6 +2220,24 @@ func Test_parseValueExpression_full(t *testing.T) {
return []any{"list", "of", "strings"}
},
},
{
name: "nested list",
valueExpression: `[{"list":[{"foo":"bar"}]}, {"bar":"baz"}]`,
expected: func() any {
return []any{
map[string]any{
"list": []any{
map[string]any{
"foo": "bar",
},
},
},
map[string]any{
"bar": "baz",
},
}
},
},
}

for _, tt := range tests {
Expand Down