Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/ottl] Change context inferrer to use functions and enums as hints #36869

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

edmocosta
Copy link
Contributor

@edmocosta edmocosta commented Dec 17, 2024

Description

This PR is part of #29017, and a spin-off from #36820. It changes the existing context inferrer logic to also take into consideration the functions and enums used on the statements. (#36820 (comment))

New logic:

  • Find all path, function names(editor, converter), and enumSymbol on the statements
  • Pick the highest priority context (same existing logic) based on the path.Context values
  • If the chosen context does not support all used functions and enums, it goes through it's lower contexts (wide scope contexts that does support the chosen context as a path context) testing them and choosing the first one that supports them.
  • If no context that can handle the functions and enums is found, the inference fail and an empty value is returned.

The parser collection was adapted to support the new context inferrer configuration requirements.

Other important changes:

Currently, it's possible to have paths to contexts root objects, for example: set(attributes["body"], resource). Given resource has no dot separators on the path, the grammar extracts it into the path.Fields slice, letting the path.Context value empty. Why? This grammar behaviour is still necessary to keep backward compatibility with paths without context, otherwise it would start requiring contexts for all paths independently of the parser configuration.

Previous PRs didn't take this edge case into consideration, and a few places needed to be changed to address it:

When/If we reach the point to deprecate paths without context, all those conditions can be removed, and the grammar changed to require and extract the path context properly.

Link to tracking issue

#29017

Testing

Unit tests

Documentation

No changes

@edmocosta edmocosta marked this pull request as ready for review December 17, 2024 14:18
@edmocosta edmocosta requested a review from a team as a code owner December 17, 2024 14:18
@edmocosta edmocosta changed the title [pkg/ottl] Changed context inferrer to use functions and enums as hints [pkg/ottl] Change context inferrer to use functions and enums as hints Dec 17, 2024
@TylerHelmuth
Copy link
Member

Currently, it's possible to have paths to contexts root objects, for example: set(attributes["body"], resource). Given resource has no dot separators on the path, the grammar extracts it into the path.Fields slice, letting the path.Context value empty. Why? This grammar behaviour is still necessary to keep backward compatibility with paths without context, otherwise it would start requiring contexts for all paths independently of the parser configuration.

ya all the context implementations allowed these paths, but the grammar didn't actually allow you to use them in any statements I don't think. I don't think we need to support a path referencing the entire object higher in the otlp payload. I am ok with removing this capability from our context implementations and making statements like set(attributes["body"], resource) fail.

pkg/ottl/functions.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/ottl Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants