Skip to content

Commit

Permalink
[pkg/ottl] Validate that all path elements are used (open-telemetry#3…
Browse files Browse the repository at this point in the history
…0042)

**Description:** 
Updates OTTL to be able to detect when a context has not used all parts
of a path and its keys. It only validates that each path/key was grabbed
- it cannot validate that the context used the value "correctly".

An example error message, taken from the unit test, looks like:

```
error while parsing arguments for call to "testing_getsetter": invalid argument at position 0: the path section "not-used" was not used by the context - this likely means you are using extra path sections
```

**Link to tracking Issue:**
Closes
open-telemetry#22744

**Testing:**
Added new unit tests

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
  • Loading branch information
2 people authored and cparkins committed Jan 10, 2024
1 parent c5443bf commit d9a1605
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 10 deletions.
27 changes: 27 additions & 0 deletions .chloggen/ottl-valid-path-used.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: breaking

# 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: Now validates against extraneous path segments or keys 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]

# (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: []
6 changes: 5 additions & 1 deletion pkg/ottl/expression.go
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,11 @@ func (p *Parser[K]) newGetter(val value) (Getter[K], error) {
return &literal[K]{value: *i}, nil
}
if eL.Path != nil {
return p.pathParser(newPath[K](eL.Path.Fields))
np, err := newPath[K](eL.Path.Fields)
if err != nil {
return nil, err
}
return p.parsePath(np)
}
if eL.Converter != nil {
return p.newGetterFromConverter(*eL.Converter)
Expand Down
62 changes: 55 additions & 7 deletions pkg/ottl/functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ type Enum int64

type EnumSymbol string

func newPath[K any](fields []field) Path[K] {
func newPath[K any](fields []field) (*basePath[K], error) {
if len(fields) == 0 {
return nil
return nil, fmt.Errorf("cannot make a path from zero fields")
}
var current *basePath[K]
for i := len(fields) - 1; i >= 0; i-- {
Expand All @@ -34,20 +34,30 @@ func newPath[K any](fields []field) Path[K] {
}
}
current.fetched = true
return current
return current, nil
}

// Path represents a chain of path parts in an OTTL statement, such as `body.string`.
// A Path has a name, and potentially a set of keys.
// If the path in the OTTL statement contains multiple parts (separated by a dot (`.`)), then the Path will have a pointer to the next Path.
type Path[K any] interface {
// Name is the name of this segment of the path.
Name() string

// Next provides the next path segment for this Path.
// Will return nil if there is no next path.
Next() Path[K]

// Key provides the Key for this Path.
// Will return nil if there is no Key.
Key() Key[K]
}

var _ Path[any] = &basePath[any]{}

type basePath[K any] struct {
name string
key Key[K]
key *baseKey[K]
nextPath *basePath[K]
fetched bool
}
Expand All @@ -65,20 +75,30 @@ func (p *basePath[K]) Next() Path[K] {
}

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
}
return p.nextPath.isComplete()
}

func newKey[K any](keys []key) Key[K] {
func newKey[K any](keys []key) *baseKey[K] {
if len(keys) == 0 {
return nil
}
Expand All @@ -90,13 +110,25 @@ func newKey[K any](keys []key) Key[K] {
nextKey: current,
}
}
current.fetched = true
return current
}

// Key represents a chain of keys in an OTTL statement, such as `attributes["foo"]["bar"]`.
// A Key has a String or Int, and potentially the next Key.
// If the path in the OTTL statement contains multiple keys, then the Key will have a pointer to the next Key.
type Key[K any] interface {
// String returns a pointer to the Key's string value.
// If the Key does not have a string value the returned value is nil.
// If Key experiences an error retrieving the value it is returned.
String(context.Context, K) (*string, error)

// Int returns a pointer to the Key's int value.
// If the Key does not have a int value the returned value is nil.
// If Key experiences an error retrieving the value it is returned.
Int(context.Context, K) (*int64, error)

// Next provides the next Key.
// Will return nil if there is no next Key.
Next() Key[K]
}

Expand Down Expand Up @@ -141,6 +173,18 @@ func (k *baseKey[K]) isComplete() error {
return k.nextKey.isComplete()
}

func (p *Parser[K]) parsePath(ip *basePath[K]) (GetSetter[K], error) {
g, err := p.pathParser(ip)
if err != nil {
return nil, err
}
err = ip.isComplete()
if err != nil {
return nil, err
}
return g, nil
}

func (p *Parser[K]) newFunctionCall(ed editor) (Expr[K], error) {
f, ok := p.functions[ed.Function]
if !ok {
Expand Down Expand Up @@ -366,7 +410,11 @@ func (p *Parser[K]) buildArg(argVal value, argType reflect.Type) (any, error) {
if argVal.Literal == nil || argVal.Literal.Path == nil {
return nil, fmt.Errorf("must be a path")
}
arg, err := p.pathParser(newPath[K](argVal.Literal.Path.Fields))
np, err := newPath[K](argVal.Literal.Path.Fields)
if err != nil {
return nil, err
}
arg, err := p.parsePath(np)
if err != nil {
return nil, err
}
Expand Down
56 changes: 54 additions & 2 deletions pkg/ottl/functions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,56 @@ func Test_NewFunctionCall_invalid(t *testing.T) {
Function: "non_pointer",
},
},
{
name: "path parts not all used",
inv: editor{
Function: "testing_getsetter",
Arguments: []argument{
{
Value: value{
Literal: &mathExprLiteral{
Path: &path{
Fields: []field{
{
Name: "name",
},
{
Name: "not-used",
},
},
},
},
},
},
},
},
},
{
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 @@ -2228,7 +2278,9 @@ func Test_newPath(t *testing.T) {
Name: "string",
},
}
p := newPath[any](fields)
np, err := newPath[any](fields)
assert.NoError(t, err)
p := Path[any](np)
assert.Equal(t, "body", p.Name())
assert.Nil(t, p.Key())
p = p.Next()
Expand Down Expand Up @@ -2375,7 +2427,7 @@ func Test_newKey(t *testing.T) {
String: ottltest.Strp("bar"),
},
}
k := newKey[any](keys)
k := Key[any](newKey[any](keys))
s, err := k.String(context.Background(), nil)
assert.NoError(t, err)
assert.NotNil(t, s)
Expand Down
8 changes: 8 additions & 0 deletions pkg/ottl/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,14 @@ 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

0 comments on commit d9a1605

Please sign in to comment.