[pkg/ottl] Update statements to reflect their context

[TylerHelmuth]

Component(s)

pkg/ottl

Is your feature request related to a problem? Please describe.

Currently OTTL statements are independent of any context. Users must tell OTTL the context in which the statement is should be run.

This results in OTTL statements not being directly portable between components. Unless the config implementing OTTL is the same between components, the OTTL statement may not be able to be copied from one component to another.

If the OTTL statement expresses the context itself then there is no ambiguity and the statement would mean the same thing in any component.

Describe the solution you'd like

Update OTTL statements to express the context, probably via the path names. When parsing the statement, infer the context to be the lowest context defined in the statement.

The Parser needs updated to be told which contexts to consider. To start, one implementation would be to try parsing the statement in each context, starting with the "highest" context. The first context to pass parsing is used.

Describe alternatives you've considered

No response

Additional context

No response

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[jsuereth]

For this issue, I was thinking of proposing the following syntax:

on <context>
(where <expr>) ?
(yield <expr> | drop)

You could then allow statements to either be what they are today, or this new syntax which carries the context.

Example:

on metric
where metric.attributes["environment"] == "qa"
drop

Note: the yield syntax is part of a larger proposal. For now you could ignore that instead just have the last part of the grammar be existing statements like Route(), Drop(), set(...).

[TylerHelmuth]

@jsuereth for context, during a working session at Kubecon we imagine the solution would be to include the context in all paths. So something like:

transform:
  error_mode: ignore
  trace_statements:
    - context: resource
      statements:
        - keep_keys(attributes, ["service.name", "service.namespace", "cloud.region", "process.command_line"])
    - context: span
      statements:
        - set(status.code, 1) where attributes["http.path"] == "/health"

becomes

transform:
  error_mode: ignore
  traces_statements:
      - keep_keys(resource.attributes, ["service.name", "service.namespace", "cloud.region", "process.command_line"])
      - set(span.status.code, 1) where span.attributes["http.path"] == "/health"

And the parser could infer the context based on the "lowest" specified context in the statement. A big benefit to this solution is the ability to build the new statements from existing config. I believe there is a way to implement this solution without incurring a breaking change, leaving both configurations available until OTTL goes stable.

[edmocosta]

Hi @TylerHelmuth!

I'm not sure if you already have an implementation in mind, but I did prototype a solution for this issue locally, and it would be nice to discuss it with you. The main changes I'm proposing would be:

Transform Processor Configuration:

For supporting both configuration styles, we could implement a custom Unmarshal function for the transformprocessor.Config that would work for flat, structured and mixed configuration styles. A few details:

• No transformprocessor.Config structure changes, statements would continue to use the same struct, but letting Context and Conditions empty when configuring using the flat format.
• For flat configurations, each statement line becomes an individual common.ContextStatements object, so we can infer their context's name later on the parser, without the need of grouping them by context, for example.
• Only the confmap.Conf would be manipulated (simulatingContextStatements and merging the config), the parsing itself would continue to be done by mapstructure.

Example of mixed configuration that would also work with the spiked Unmarshal function:

trace_statements:
  - set(span.status.code, 1) where span.attributes["http.path"] == "/health"
  - set(span.status.code, 1) where span.attributes["http.path"] == "/health"
  - context: spanevent
     statements:
       - set(attributes["domain"], "test.com")

OTTL contexts:

Given the context name would become part of the path, all OTTL contexts parsePath functions would need to be changed to ignore it, for example, the ottllog parsePath should contain an extra condition comparing the first path.Name() with the context name, skipping it to path.Next() if it matches.

An alternative to skipping the path's context would be changing the grammar to extract the context name to another field, which demonstrated to be much more complex giving that valid contexts names vary depending on the parent's context. In addition to that, It wouldn't be useful to infer the context, as the transform processor needs to know it beforehand to then execute the proper parser.

Expressing that on participle is also challenging. If we had a different syntax in mind, e.g. the suggested on metric .., it probably would be easier.

That extra context condition wouldn't affect other processor's OTTL usages (routingprocessor, filterprocessor, etc). I couldn't find any context/field name conflicts at this time (e.g. log context supporting log field), so it wouldn't skip anything for existing usages.

Transform processor:

*.ParseContextStatements functions should be changed to infer the context when necessary (.Context == ""),
It could be done by trying to parse it in each context or be using some shared inferrer implementation.

Considering we know at parsing time which contexts names are valid, and that each flat configuration statement line becomes a separate common.ContextStatements object, this inferrer implementation could just regex the valid contexts names from the statements, picking the highest one. For example:

For the given configuration:

trace_statements:
 - keep_keys(resource.attributes, ["service.name", "service.namespace", "cloud.region", "process.command_line"])
 - set(span.status.code, 1) where spanevent.attributes["http.path"] == "/health"

It would use pre-compiled regexes (per statement type) to find all valid context names on the statement. For this trace_statements example, it would use something like:

(?:[^"])(?P<context>span|spanevent|resource|scope)(?:\.(?P<field>\w+))

Given the above regex captured context names, it would infer the resource context for the first statement, and spanevent for the second one, as spanevent > span.

Trying parsing the statement in each context would probably be easier & safer, but in terms of performance, I think the regex-based infer mechanism - although not optimal - would be faster depending on the statement. For example, parsing a statement with a lower context set(span.status.code, 1) where span.attributes["http.path"] == "/health" would perform as:

BenchmarkParseContextStatements-10                      16578	    72180 ns/op	   55083 B/op	     729 allocs/op
BenchmarkParseContextStatementsRegexInferred-10         16192	    73589 ns/op	   55571 B/op	     736 allocs/op
BenchmarkParseContextStatementsTwoCtxAttempts-10         8635	   135392 ns/op	   93686 B/op	    1292 allocs/op

As you can see on the benchmark, the regex has an overhead, but it's still much smaller than trying to parse the statement in another context (spanevent, then span), unless it succeeds at the first attempt, but I'm not sure how common that would be. WDYT?

---

As I mentioned, I've ran this proposal code locally and it seems to be working fine so far, I'm still testing it, and I'm probably missing some points. It would be great if you could give it a thought and share your opinion. I'd be happy to implement any other idea you might have in mind.

Thank you!

[TylerHelmuth]

@edmocosta thanks for the excellent investigation into this issue.

An alternative to skipping the path's context would be changing the grammar to extract the context name to another field

The original discussion we had at Kubecon NA 2023 was that we do want to build this concept into the grammar. We liked this idea because it makes OTTL syntax consistent everywhere. One of the problems today is that an OTTL statement (or condition) cannot be copy and pasted from 1 component to another unless you're use the context is the same. There is also nothing enforcing to components express the desired context in the same way. Requiring the context as part of the grammar would solve that problem.

Another reason we liked this idea is it made paths more readable. When the current context is defined somewhere else in the config (or even the statement), then a reader must remember what the current context is when reading a path that has not prefix. But if the context must always prefix a path, then there is no ambiguity.

As for the technical implementation I believe it is possible to add all this business logic somewhere in the parser/ottlcontext and not the grammar itself. I believe our idea was something like "give a path has been parsed, the first part MUST be the context. If a path is only 1 part, it is invalid", or something like that.

An actual change to the grammar is probably a better solution bc we could enforce all paths have a context and capture it in its own field

type path struct {
        Context string `parser:"@Lowercase '.'"`
	Fields []field `parser:"@@ ( '.' @@ )*"`
}

Or something like that.

In addition to that, It wouldn't be useful to infer the context, as the transform processor needs to know it beforehand to then execute the proper parser.

I believe this issue could be solved by adding some more features to OTTL's package. At Kubecon we discussed a brute force solution (because it happens at startup) where the parser infers the context to be the "lowest" context defined in the statement. In addition, the parser would need to be told which contexts to consider (and their order), and it would try parsing at the "highest" context, returning the parsed statements for the fist context that was able to successfully parse all the statements. It would probably look pretty similar to the transformprocessor's existing ParserCollection concept.

Using regex to identify the concept also seems reasonable, but I believe it should be a OTTL concern, not a component concern. Performance will end up being a secondary concern since this is all happening at startup. I like the concept of not needing to brute force to check which context should be used.

Example of mixed configuration that would also work with the spiked Unmarshal function:

I really like the sound of this idea, at least for some amount of transition time. Our goal with this change was to try and not break existing OTTL users - we figured we'd be able to build the new statements using the existing config structs. It sounds like you've confirmed this is possible.

As for breaking changes in OTTL's public API, we can do those all we want.

If you're interested in continuing this work, I am very interested in seeing what it is like to implement a solution via the grammar instead of via the transformprocessor.

[edmocosta]

Hi @TylerHelmuth!

As for the technical implementation I believe it is possible to add all this business logic somewhere in the parser/ottlcontext and not the grammar itself. I believe our idea was something like "give a path has been parsed, the first part MUST be the context. If a path is only 1 part, it is invalid", or something like that.

An actual change to the grammar is probably a better solution bc we could enforce all paths have a context and capture it in its own field

Yes, I completely agree with that idea, it's simpler to read and to understand and I think that's the way to go.
Regarding the technical details, my only concern on that is backward compatibility, if we capture/enforce the first path's segment to be the context name, it would probably break existing configurations, as paths like attributes["foo"] and status.code would be considered invalid, failing on the validation, and/or being parsed as {Context: "status", Name: "code", Next: <nil>} (causing invalid path segment errors on the context's parsePath functions).

That's definitely not a blocker for implementing the proposed solution in a BC way, during the transition time, we could maybe handle it on the parser, and instead of using the valid contexts list to validate the path prefix, we could use it to temporary keep BC, changing the parser newPath method adding something like:

fields := path.Fields
...
if path.Context != "" {
  if _, ok := p.validContexts[path.Context]; !ok {
    // if the context is not recognised, it's likely to be part of the field name (old structure without context).
    // when the transition is complete, we can just return an error here.
    fields = append([]field{{Name: path.Context}}, fields...)

    // maybe log some warn/deprecation message about paths without context?
  }
}

Pros of changing this function instead of adding the logic into the ottlcontexts are backward compatibility from the source (parser), no need to change the OTTL context's parsePath functions, and easily switch it to require/validate the path context name when the transition time is considered complete.

On the other other hand, the components consuming those paths would still need to handle both versions, with and without the context prefix. No validations would happen at the transition time.

Do you have any thoughts on this and how it should be handled?

It would probably look pretty similar to the transformprocessor's existing ParserCollection concept.

I see, that makes sense! As you mentioned, I think we could come up with a very similar ParserCollection implementation in the ottl package (ottl/parsercollection perhaps), but a bit more generic, supporting parser collections for logs, metrics and traces contexts, and handling the context inferring when necessary. It could also be used to replace all the existing transformprocessor's ParserCollections.

I'm not sure yet if we can implement it totally generic, without any dependency on the ottlcontexts for example (probably not). Either way, IMO, having the ottlcontext as a dependency wouldn't be an issue considering the parser collection's purpose. I'll investigate it further during the implementation.

Using regex to identify the concept also seems reasonable, but I believe it should be a OTTL concern, not a component concern. Performance will end up being a secondary concern since this is all happening at startup. I like the concept of not needing to brute force to check which context should be used.

Agreed.

I really like the sound of this idea, at least for some amount of transition time. Our goal with this change was to try and not break existing OTTL users - we figured we'd be able to build the new statements using the existing config structs. It sounds like you've confirmed this is possible.

Correct! that's possible.

---

Please let me know your thoughts!

Thanks!

[TylerHelmuth]

it would probably break existing configurations, as paths like attributes["foo"] and status.code would be considered invalid, failing on the validation, and/or being parsed as {Context: "status", Name: "code", Next: } (causing invalid path segment errors on the context's parsePath functions).

I agree that a change in the grammar would be a breaking change for parsing a statement like set(attributes["foo"], "bar"). I think a way around this is for component that are using OTTL to modify the user's configured string using the configured context. So for the transform processor, if a user configured

transform:
  trace_statements:
    - context: span
      statements:
        - set(attributes["test"], "pass")

Then the string passed into the parser would be changed to set(span.attributes["test"], "pass"). I believe another change we'd need is to create some sort of wrapper that would obfuscate the Parser types from callers. So instead of using the context value to determine that ottlspan.NewParser needs to be invoked, we'd pass the statements to the wrapper and it would figure out which parser needs invoked.

If we can modify the input string using the configured context then I believe we can do this change with no breaking changes to user config. There will definitely be breaking changes to OTTL APIs in places, but we aren't worried about breaking those.

With this strategy components can then expose support for both the old style and the new style allowing a transition period. The component could even print out the modified statements as a way to help users transition.

An additional idea to play around with would be making the context selector in the grammar optional. This may allow both old and new syntax to be parsed by the same grammar, allowing us to make a bunch of additions/changes to pkg/ottl's API without also needing to update all the components in the same PR. The more work we can do in small batches the better (but that typically has been difficult when breaking OTTL's grammar/API).

Pros of changing this function instead of adding the logic into the ottlcontexts are backward compatibility from the source (parser), no need to change the OTTL context's parsePath functions, and easily switch it to require/validate the path context name when the transition time is considered complete.

I would definitely love to avoid having to change parsePath functions. I think adding the context selector to the grammar allows us to keep that business logic in the parser wrapper helper thing and the contexts can continue to focus only on the path.

[edmocosta]

Hi @TylerHelmuth!

Thank you again for your patience and support, I'm still getting up to speed with the OTel codebase/way of working/standards/etc, so apologise for so many questions and perhaps suggestions that might not make much sense :)

An additional idea to play around with would be making the context selector in the grammar optional. This may allow both old and new syntax to be parsed by the same grammar, allowing us to make a bunch of additions/changes to pkg/ottl's API without also needing to update all the components in the same PR.

I forgot to mention it, but that was the idea behind my previous suggestion. If we make the Context grammar selector optional by adding it as following:

// path represents a telemetry path mathExpression.
type path struct {
	Context string  `parser:"(@Lowercase '.')?"`
	Fields  []field `parser:"@@ ( '.' @@ )*"`
}

We would be able to make it non-breaking to all components - without modifying them - by changing the ottl.Parser[k].newPath function as following:

func (p *Parser[K]) newPath(path *path) (*basePath[K], error) {
	fields := path.Fields
	...
	if path.Context != "" {
	   if p.validContexts == nil || len(p.validPathContexts) == 0 {
              fields = append([]field{{Name: path.Context}}, fields...)
	   } else {
	      if _, ok := p.validContexts[path.Context]; !ok {
		return nil, fmt.Errorf("invalid path context %s.%s: %[1]s", path.Context, originalText)
	     }
	   }
        }
        ...
}

Given that all existing ottl.Parser[K] usages are not setting the parser option WithValidContexts (to-be-added), it would automatically fallback to the previous behaviour, with the grammar's extracted Context value as the first path segment.

I might be wrong, but I think this should be enough to keep the parser and grammar backward compatible, and we could introduce these changes in an individual PR.

---

The next step would be building the statements rewrite utility, so components would be able to change their config text adding the missing path's context. I'm still unsure how this utility would be implemented: using regex then appending the context perhaps, or maybe parsing the statements text into the grammar AST, changing the paths, and then parsing it back to string (somehow). Any ideas on this?

Alternatively to the rewrite utility, when I mentioned

No validations would happen at the transition time.

I meant (coded like this to compare to the above version):

func (p *Parser[K]) newPath(path *path) (*basePath[K], error) {
	fields := path.Fields
	...
	if path.Context != "" {
	   if p.validContexts == nil || len(p.validContexts) == 0 {
              fields = append([]field{{Name: path.Context}}, fields...)
	   } else {
	      if _, ok := p.validContexts[path.Context]; !ok {
-		  return nil, fmt.Errorf("invalid path context %s.%s: %[1]s", path.Context, originalText)
+                 fields = append([]field{{Name: path.Context}}, fields...)
	     }
	   }
        }
        ...
}

This change would make the parser support both versions, with and without the path's context prefix. When we finish migrating everything, we could change it back to the validation mode.

Please note that the p.validContexts would normally hold a single context name, for example, the ottllog Parser would set it to log, ottlspan to span, and so on.

---

Finally, we would need to add the parser wrapper implementation, that would infer the context from the statements, pick the right parser, and execute the parse statements operation.

At this point, we could introduce an extra parser config for complementing the WithValidContexts option, enabling the validation mode on demand, and instead of falling back to the previous behaviour as suggested above, enforcing all paths to have a valid context. For example:

p, err := ottl.NewParser[ottllog.TransformContext](
   	functions,
   	pep.parsePath,
   	telemetrySettings,
   	ottl.WithEnumParser[ottllog.TransformContext](parseEnum),
   	ottl.WithValidContexts[ottllog.TransformContext]([]string{"log"})
   	ottl.RequirePathsContext[ottllog.TransformContext](),
   )

---

Last change would be on the transformprocessor, adapating the configuration as discussed, and using the new parsers wrapper and the statement rewrite utilities.

[TylerHelmuth]

I might be wrong, but I think this should be enough to keep the parser and grammar backward compatible, and we could introduce these changes in an individual PR.

I am definitely interested in trying this approach.

The next step would be building the statements rewrite utility, so components would be able to change their config text adding the missing path's context. I'm still unsure how this utility would be implemented: using regex then appending the context perhaps, or maybe parsing the statements text into the grammar AST, changing the paths, and then parsing it back to string (somehow). Any ideas on this?

I think it is solvable using regex and some assumptions. Like if we know the context configured is span, then any path that doesn't start with reasource or instrumentation_scope needs span appended to it. I think a regex to identify a path in a statement should be doable.

This change would make the parser support both versions, with and without the path's context prefix. When we finish migrating everything, we could change it back to the validation mode.

I think this would also work, but I'd really like to be able to print out for the user what the new statement syntax should be to help them transition. Maybe the ottlcontext could do that instead of the transformprocessor manipulating the input and printing it?

Finally, we would need to add the parser wrapper implementation, that would infer the context from the statements, pick the right parser, and execute the parse statements operation.

This part is still the most hazy aspect of this change in my mind, but if you have and idea I'm looking forward to see it implemented.

@edmocosta at this point I'm interested in seeing an implementation - it doesn't need to pass tests and can include as many breaking changes as you want.