From f9807d0b92a9f7e1fa398f2bdcb75fc48110daa9 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 18 Dec 2023 15:49:09 -0700 Subject: [PATCH] [pkg/ottl] Don't check that all keys are used (#30047) **Description:** The desire to validate both path segments AND keys led to a bug where a totally valid statement like `replace_match(body["metadata"]["uid"], "*", "12345")` fails since only `metadata` is checked during parsing; `uid` is checked during hot-path get/set of the value. Failing such a statement is not the intention of https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/22744 and it was incorrect to fail such a statement. In fact, I believe validating the key's use in the context will be even more complex once we introduce dynamic indexing. For these reasons, this PR removes Key validation for now. If, in the future, we want to re-add these validations, our Interfaces allow that. **Link to tracking Issue:** https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/22744 https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/30051 **Testing:** Unit tests Also we wouldve caught this issue earlier if we had an e2e test that did complex indexing but unfortunately we did in the transform processor. All the more reason to implement https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/28642. --- .chloggen/ottl-valid-path-used.yaml | 2 +- pkg/ottl/functions.go | 25 --- pkg/ottl/functions_test.go | 156 ++++-------------- pkg/ottl/parser_test.go | 8 - .../internal/logs/processor_test.go | 4 + 5 files changed, 35 insertions(+), 160 deletions(-) diff --git a/.chloggen/ottl-valid-path-used.yaml b/.chloggen/ottl-valid-path-used.yaml index 18baeafe777cc..b7cb63d491dc6 100755 --- a/.chloggen/ottl-valid-path-used.yaml +++ b/.chloggen/ottl-valid-path-used.yaml @@ -7,7 +7,7 @@ change_type: breaking component: pkg/ottl # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Now validates against extraneous path segments or keys that a context does not know how to use. +note: Now validates against extraneous path segments that a context does not know how to use. # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. issues: [30042] diff --git a/pkg/ottl/functions.go b/pkg/ottl/functions.go index d7872ef4082f5..6a10b915efe31 100644 --- a/pkg/ottl/functions.go +++ b/pkg/ottl/functions.go @@ -78,7 +78,6 @@ func (p *basePath[K]) Key() Key[K] { if p.key == nil { return nil } - p.key.fetched = true return p.key } @@ -86,12 +85,6 @@ func (p *basePath[K]) isComplete() error { if !p.fetched { return fmt.Errorf("the path section %q was not used by the context - this likely means you are using extra path sections", p.name) } - if p.key != nil { - err := p.key.isComplete() - if err != nil { - return err - } - } if p.nextPath == nil { return nil } @@ -138,7 +131,6 @@ type baseKey[K any] struct { s *string i *int64 nextKey *baseKey[K] - fetched bool } func (k *baseKey[K]) String(_ context.Context, _ K) (*string, error) { @@ -153,26 +145,9 @@ func (k *baseKey[K]) Next() Key[K] { if k.nextKey == nil { return nil } - k.nextKey.fetched = true return k.nextKey } -func (k *baseKey[K]) isComplete() error { - if !k.fetched { - var val any - if k.s != nil { - val = *k.s - } else if k.i != nil { - val = *k.i - } - return fmt.Errorf("the key %q was not used by the context during indexing", val) - } - if k.nextKey == nil { - return nil - } - return k.nextKey.isComplete() -} - func (p *Parser[K]) parsePath(ip *basePath[K]) (GetSetter[K], error) { g, err := p.pathParser(ip) if err != nil { diff --git a/pkg/ottl/functions_test.go b/pkg/ottl/functions_test.go index 55dd4dd904a88..fe9c309fecb57 100644 --- a/pkg/ottl/functions_test.go +++ b/pkg/ottl/functions_test.go @@ -417,32 +417,6 @@ func Test_NewFunctionCall_invalid(t *testing.T) { }, }, }, - { - name: "path keys not all used", - inv: editor{ - Function: "testing_getsetter", - Arguments: []argument{ - { - Value: value{ - Literal: &mathExprLiteral{ - Path: &path{ - Fields: []field{ - { - Name: "name", - Keys: []key{ - { - String: ottltest.Strp("not-used"), - }, - }, - }, - }, - }, - }, - }, - }, - }, - }, - }, } for _, tt := range tests { @@ -1441,6 +1415,36 @@ func Test_NewFunctionCall(t *testing.T) { }, want: nil, }, + { + name: "Complex Indexing", + inv: editor{ + Function: "testing_getsetter", + Arguments: []argument{ + { + Value: value{ + Literal: &mathExprLiteral{ + Path: &path{ + Fields: []field{ + { + Name: "name", + Keys: []key{ + { + String: ottltest.Strp("foo"), + }, + { + String: ottltest.Strp("bar"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + want: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2318,106 +2322,6 @@ func Test_baseKey_Next(t *testing.T) { assert.Nil(t, next.Next()) } -func Test_baseKey_isComplete(t *testing.T) { - tests := []struct { - name string - p baseKey[any] - expectedError bool - }{ - { - name: "fetched no next", - p: baseKey[any]{ - fetched: true, - }, - }, - { - name: "fetched with next", - p: baseKey[any]{ - fetched: true, - nextKey: &baseKey[any]{ - fetched: true, - }, - }, - }, - { - name: "not fetched no next", - p: baseKey[any]{ - fetched: false, - }, - expectedError: true, - }, - { - name: "not fetched with next", - p: baseKey[any]{ - fetched: true, - nextKey: &baseKey[any]{ - fetched: false, - }, - }, - expectedError: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.p.isComplete() - if tt.expectedError { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } - }) - } -} - -func Test_baseKey_NextWithIsComplete(t *testing.T) { - tests := []struct { - name string - keyFunc func() *baseKey[any] - expectedError bool - }{ - { - name: "fetched", - keyFunc: func() *baseKey[any] { - bk := baseKey[any]{ - fetched: true, - nextKey: &baseKey[any]{ - fetched: false, - }, - } - bk.Next() - return &bk - }, - }, - { - name: "not fetched enough", - keyFunc: func() *baseKey[any] { - bk := baseKey[any]{ - fetched: true, - nextKey: &baseKey[any]{ - fetched: false, - nextKey: &baseKey[any]{ - fetched: false, - }, - }, - } - bk.Next() - return &bk - }, - expectedError: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.keyFunc().isComplete() - if tt.expectedError { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } - }) - } -} - func Test_newKey(t *testing.T) { keys := []key{ { diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index 24b126f2c8cfc..fa09f32530775 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -1236,14 +1236,6 @@ func Test_parseCondition_full(t *testing.T) { func testParsePath[K any](p Path[K]) (GetSetter[any], error) { if p != nil && (p.Name() == "name" || p.Name() == "attributes") { - - if bp, ok := p.(*basePath[K]); ok { - if bp.key != nil && bp.key.s != nil && *bp.key.s == "foo" && bp.key.nextKey != nil && bp.key.nextKey.s != nil && *bp.key.nextKey.s == "bar" { - bp.key.fetched = true - bp.key.nextKey.fetched = true - } - } - return &StandardGetSetter[any]{ Getter: func(ctx context.Context, tCtx any) (any, error) { return tCtx, nil diff --git a/processor/transformprocessor/internal/logs/processor_test.go b/processor/transformprocessor/internal/logs/processor_test.go index 519e57ca65bba..5bceea2285c88 100644 --- a/processor/transformprocessor/internal/logs/processor_test.go +++ b/processor/transformprocessor/internal/logs/processor_test.go @@ -329,6 +329,10 @@ func Test_ProcessLogs_LogContext(t *testing.T) { td.ResourceLogs().At(0).ScopeLogs().At(0).LogRecords().At(0).Attributes().PutDouble("test", 0.0) }, }, + { + statement: `replace_match(body["metadata"]["uid"], "*", "12345")`, + want: func(td plog.Logs) {}, + }, } for _, tt := range tests {