Skip to content

Commit

Permalink
[pkg/ottl] Don't check that all keys are used (open-telemetry#30047)
Browse files Browse the repository at this point in the history
**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.
  • Loading branch information
TylerHelmuth committed Dec 18, 2023
1 parent dc832e0 commit f9807d0
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 160 deletions.
2 changes: 1 addition & 1 deletion .chloggen/ottl-valid-path-used.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
25 changes: 0 additions & 25 deletions pkg/ottl/functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand Down
156 changes: 30 additions & 126 deletions pkg/ottl/functions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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{
{
Expand Down
8 changes: 0 additions & 8 deletions pkg/ottl/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions processor/transformprocessor/internal/logs/processor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit f9807d0

Please sign in to comment.