From 5e16669850bab9364d0d3be46e1d265b531427f0 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Mon, 20 Jan 2025 13:14:23 +0100 Subject: [PATCH 1/8] adapt return types of mapGetter to raw, and ValueExpression getter to pcommon types Signed-off-by: Florian Bacher --- pkg/ottl/expression.go | 9 ++++----- pkg/ottl/parser.go | 27 ++++++++++++++++++++++++++- pkg/ottl/parser_test.go | 18 +++++++++++++++--- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/pkg/ottl/expression.go b/pkg/ottl/expression.go index a2e8d29e63c4..5576231c42c6 100644 --- a/pkg/ottl/expression.go +++ b/pkg/ottl/expression.go @@ -193,11 +193,10 @@ func (m *mapGetter[K]) Get(ctx context.Context, tCtx K) (any, error) { evaluated[k] = t } } - result := pcommon.NewMap() - if err := result.FromRaw(evaluated); err != nil { - return nil, err - } - return result, nil + // return the raw mao instead of creating a pcommon.Map, + // otherwise map structures cannot be used within slices, as the Slice.FromRaw() method + // only supports raw types for its items + return evaluated, nil } // TypeError represents that a value was not an expected type. diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index ad42f9e0b327..3d44d56ef0ae 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "go.opentelemetry.io/collector/pdata/pcommon" "sort" "strings" @@ -480,6 +481,30 @@ func (p *Parser[K]) ParseValueExpression(raw string) (*ValueExpression[K], error } return &ValueExpression[K]{ - getter: getter, + getter: &StandardGetSetter[K]{ + Getter: func(ctx context.Context, tCtx K) (any, error) { + res, err := getter.Get(ctx, tCtx) + if err != nil { + return nil, err + } + + switch v := res.(type) { + case map[string]any: + m := pcommon.NewMap() + if err := m.FromRaw(v); err != nil { + return nil, err + } + return m, nil + case []any: + s := pcommon.NewSlice() + if err := s.FromRaw(v); err != nil { + return nil, err + } + return s, nil + default: + return res, nil + } + }, + }, }, nil } diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index e7ef0ccbfc7d..20b3ba9b966c 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -2151,11 +2151,15 @@ func Test_parseValueExpression_full(t *testing.T) { name: "resolve context value", valueExpression: `attributes`, expected: func() any { - return map[string]any{ + raw := map[string]any{ "attributes": map[string]any{ "foo": "bar", }, } + + m := pcommon.NewMap() + _ = m.FromRaw(raw) + return m }, tCtx: map[string]any{ "attributes": map[string]any{ @@ -2192,10 +2196,14 @@ func Test_parseValueExpression_full(t *testing.T) { name: "hex values", valueExpression: `[0x0000000000000000, 0x0000000000000000]`, expected: func() any { - return []any{ + raw := []any{ []uint8{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, []uint8{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, } + + s := pcommon.NewSlice() + _ = s.FromRaw(raw) + return s }, }, { @@ -2220,7 +2228,11 @@ func Test_parseValueExpression_full(t *testing.T) { name: "string list", valueExpression: `["list", "of", "strings"]`, expected: func() any { - return []any{"list", "of", "strings"} + raw := []any{"list", "of", "strings"} + + s := pcommon.NewSlice() + _ = s.FromRaw(raw) + return s }, }, } From af876fe3f9e3db39e9a3c4833b219cd388ef2599 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Wed, 22 Jan 2025 08:33:38 +0100 Subject: [PATCH 2/8] return raw types in mapGetter Signed-off-by: Florian Bacher --- pkg/ottl/expression_test.go | 12 +++++++++++- pkg/ottl/parser.go | 27 +-------------------------- pkg/ottl/parser_test.go | 30 +++++++----------------------- 3 files changed, 19 insertions(+), 50 deletions(-) diff --git a/pkg/ottl/expression_test.go b/pkg/ottl/expression_test.go index e5cffbfcd547..46f3291fbdd2 100644 --- a/pkg/ottl/expression_test.go +++ b/pkg/ottl/expression_test.go @@ -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")}, + }, + }, + }, + }, }, }, }, @@ -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), diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index 3d44d56ef0ae..ad42f9e0b327 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -7,7 +7,6 @@ import ( "context" "errors" "fmt" - "go.opentelemetry.io/collector/pdata/pcommon" "sort" "strings" @@ -481,30 +480,6 @@ func (p *Parser[K]) ParseValueExpression(raw string) (*ValueExpression[K], error } return &ValueExpression[K]{ - getter: &StandardGetSetter[K]{ - Getter: func(ctx context.Context, tCtx K) (any, error) { - res, err := getter.Get(ctx, tCtx) - if err != nil { - return nil, err - } - - switch v := res.(type) { - case map[string]any: - m := pcommon.NewMap() - if err := m.FromRaw(v); err != nil { - return nil, err - } - return m, nil - case []any: - s := pcommon.NewSlice() - if err := s.FromRaw(v); err != nil { - return nil, err - } - return s, nil - default: - return res, nil - } - }, - }, + getter: getter, }, nil } diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index 20b3ba9b966c..4efec212ebcc 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -13,12 +13,10 @@ import ( "time" "github.com/alecthomas/participle/v2/lexer" + "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest" "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" ) // This is not in ottltest because it depends on a type that's a member of OTTL. @@ -2151,15 +2149,11 @@ func Test_parseValueExpression_full(t *testing.T) { name: "resolve context value", valueExpression: `attributes`, expected: func() any { - raw := map[string]any{ + return map[string]any{ "attributes": map[string]any{ "foo": "bar", }, } - - m := pcommon.NewMap() - _ = m.FromRaw(raw) - return m }, tCtx: map[string]any{ "attributes": map[string]any{ @@ -2196,14 +2190,10 @@ func Test_parseValueExpression_full(t *testing.T) { name: "hex values", valueExpression: `[0x0000000000000000, 0x0000000000000000]`, expected: func() any { - raw := []any{ + return []any{ []uint8{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, []uint8{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, } - - s := pcommon.NewSlice() - _ = s.FromRaw(raw) - return s }, }, { @@ -2217,22 +2207,16 @@ 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), + } }, }, { name: "string list", valueExpression: `["list", "of", "strings"]`, expected: func() any { - raw := []any{"list", "of", "strings"} - - s := pcommon.NewSlice() - _ = s.FromRaw(raw) - return s + return []any{"list", "of", "strings"} }, }, } From 4d8ab84023dc81fc2764ece7cf5813c33e762ba0 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Wed, 22 Jan 2025 08:43:55 +0100 Subject: [PATCH 3/8] add e2e test and changelog entry Signed-off-by: Florian Bacher --- .chloggen/ottl-nested-map-literals.yaml | 27 +++++++++++++++++++++++++ pkg/ottl/e2e/e2e_test.go | 8 ++++++++ 2 files changed, 35 insertions(+) create mode 100644 .chloggen/ottl-nested-map-literals.yaml diff --git a/.chloggen/ottl-nested-map-literals.yaml b/.chloggen/ottl-nested-map-literals.yaml new file mode 100644 index 000000000000..80f79eab4432 --- /dev/null +++ b/.chloggen/ottl-nested-map-literals.yaml @@ -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 slices 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: [] diff --git a/pkg/ottl/e2e/e2e_test.go b/pkg/ottl/e2e/e2e_test.go index e803a7bb3092..f25dfb426186 100644 --- a/pkg/ottl/e2e/e2e_test.go +++ b/pkg/ottl/e2e/e2e_test.go @@ -1041,6 +1041,14 @@ 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") + }, + }, } for _, tt := range tests { From d21702e498f35ca3a9d7a64b7bc27ef5a438a2f1 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Wed, 22 Jan 2025 09:39:57 +0100 Subject: [PATCH 4/8] fix linting Signed-off-by: Florian Bacher --- pkg/ottl/parser_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index 4efec212ebcc..9d57462a132e 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -13,10 +13,11 @@ import ( "time" "github.com/alecthomas/participle/v2/lexer" - "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component/componenttest" + + "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest" ) // This is not in ottltest because it depends on a type that's a member of OTTL. From 659705eafd5754d5b8ac43917ba9f180a7f8f73b Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Jan 2025 10:13:22 +0100 Subject: [PATCH 5/8] keep returning pcommon.Map in mapGetter, handle edge case in listGetter instead Signed-off-by: Florian Bacher --- pkg/ottl/e2e/e2e_test.go | 8 ++++++++ pkg/ottl/expression.go | 20 +++++++++++++++----- pkg/ottl/parser.go | 16 +++++++++++++++- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/pkg/ottl/e2e/e2e_test.go b/pkg/ottl/e2e/e2e_test.go index f25dfb426186..bcb69fe9a30a 100644 --- a/pkg/ottl/e2e/e2e_test.go +++ b/pkg/ottl/e2e/e2e_test.go @@ -1049,6 +1049,14 @@ func Test_e2e_converters(t *testing.T) { 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") + }, + }, } for _, tt := range tests { diff --git a/pkg/ottl/expression.go b/pkg/ottl/expression.go index 5576231c42c6..3f7e5f22bb55 100644 --- a/pkg/ottl/expression.go +++ b/pkg/ottl/expression.go @@ -169,7 +169,15 @@ func (l *listGetter[K]) Get(ctx context.Context, tCtx K) (any, error) { if err != nil { return nil, err } - evaluated[i] = val + switch t := val.(type) { + case pcommon.Map: + // convert pcommon.Map items within the list to map[string]any to avoid + // errors due to pcommon.Map not being handled in the pcommon.Value.FromRaw() + // method. This can happen if we have a map containing a list containing a map, e.g. {"list":[{"foo":"bar"}]} + evaluated[i] = t.AsRaw() + default: + evaluated[i] = val + } } return evaluated, nil @@ -193,10 +201,12 @@ func (m *mapGetter[K]) Get(ctx context.Context, tCtx K) (any, error) { evaluated[k] = t } } - // return the raw mao instead of creating a pcommon.Map, - // otherwise map structures cannot be used within slices, as the Slice.FromRaw() method - // only supports raw types for its items - return evaluated, nil + + newMap := pcommon.NewMap() + if err := newMap.FromRaw(evaluated); err != nil { + return nil, err + } + return newMap, nil } // TypeError represents that a value was not an expected type. diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index ad42f9e0b327..eeb57619b8fe 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "go.opentelemetry.io/collector/pdata/pcommon" "sort" "strings" @@ -480,6 +481,19 @@ func (p *Parser[K]) ParseValueExpression(raw string) (*ValueExpression[K], error } return &ValueExpression[K]{ - getter: getter, + getter: &StandardGetSetter[K]{ + 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 + default: + return v, nil + } + }, + }, }, nil } From 09518c1f332469515140ef481b4378e62b5c96b0 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Jan 2025 10:58:21 +0100 Subject: [PATCH 6/8] fix linting Signed-off-by: Florian Bacher --- pkg/ottl/parser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index eeb57619b8fe..f0a5ff665b96 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -7,12 +7,12 @@ import ( "context" "errors" "fmt" - "go.opentelemetry.io/collector/pdata/pcommon" "sort" "strings" "github.com/alecthomas/participle/v2" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/pdata/pcommon" "go.uber.org/zap" ) From a376ab4c5eb830ae1dc3e4c242db46b81d0bb413 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Jan 2025 12:53:52 +0100 Subject: [PATCH 7/8] apply suggestion from code review Signed-off-by: Florian Bacher --- pkg/ottl/expression.go | 44 ++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/pkg/ottl/expression.go b/pkg/ottl/expression.go index 3f7e5f22bb55..a3d5562d9f22 100644 --- a/pkg/ottl/expression.go +++ b/pkg/ottl/expression.go @@ -169,15 +169,8 @@ func (l *listGetter[K]) Get(ctx context.Context, tCtx K) (any, error) { if err != nil { return nil, err } - switch t := val.(type) { - case pcommon.Map: - // convert pcommon.Map items within the list to map[string]any to avoid - // errors due to pcommon.Map not being handled in the pcommon.Value.FromRaw() - // method. This can happen if we have a map containing a list containing a map, e.g. {"list":[{"foo":"bar"}]} - evaluated[i] = t.AsRaw() - default: - evaluated[i] = val - } + + evaluated[i] = val } return evaluated, nil @@ -188,25 +181,38 @@ 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 val.(type) { case pcommon.Map: - evaluated[k] = t.AsRaw() + target := result.PutEmpty(k).SetEmptyMap() + val.(pcommon.Map).CopyTo(target) + case []any: + target := result.PutEmpty(k).SetEmptySlice() + for _, el := range val.([]any) { + switch el.(type) { + case pcommon.Map: + m := target.AppendEmpty().SetEmptyMap() + el.(pcommon.Map).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 + } } } - - newMap := pcommon.NewMap() - if err := newMap.FromRaw(evaluated); err != nil { - return nil, err - } - return newMap, nil + return result, nil } // TypeError represents that a value was not an expected type. From 38a56d6e0fd0615b125956ff84b901d2221e91fa Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 23 Jan 2025 13:10:53 +0100 Subject: [PATCH 8/8] fix linting Signed-off-by: Florian Bacher --- pkg/ottl/expression.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/ottl/expression.go b/pkg/ottl/expression.go index a3d5562d9f22..62ba64bc9806 100644 --- a/pkg/ottl/expression.go +++ b/pkg/ottl/expression.go @@ -187,17 +187,17 @@ func (m *mapGetter[K]) Get(ctx context.Context, tCtx K) (any, error) { if err != nil { return nil, err } - switch val.(type) { + switch typedVal := val.(type) { case pcommon.Map: target := result.PutEmpty(k).SetEmptyMap() - val.(pcommon.Map).CopyTo(target) + typedVal.CopyTo(target) case []any: target := result.PutEmpty(k).SetEmptySlice() - for _, el := range val.([]any) { - switch el.(type) { + for _, el := range typedVal { + switch typedEl := el.(type) { case pcommon.Map: m := target.AppendEmpty().SetEmptyMap() - el.(pcommon.Map).CopyTo(m) + typedEl.CopyTo(m) default: err := target.AppendEmpty().FromRaw(el) if err != nil {