[pkg/ottl] Don't check that all keys are used (open-telemetry#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 open-telemetry#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:** open-telemetry#22744 open-telemetry#30051 **Testing:** <Describe what testing was performed and which tests were added.> 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 open-telemetry#28642.
AmadeusITGroup · Jan 10, 2024 · fbdc8ee · fbdc8ee
1 parent 3a9cfbd
commit fbdc8ee
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 160 deletions.
diff --git 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
@@ -78,20 +78,13 @@ func (p *basePath[K]) Key() Key[K] {
 	if p.key == nil {
 		return nil
 	}
-	p.key.fetched = true
 	return p.key
 }
 
 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
@@ -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
@@ -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
@@ -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 {