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.

Sign up for GitHub

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

Jump to bottom

[pkg/ottl] Validate that all parts of a Key are used by a Context #30051

Closed
Tracked by #28892
TylerHelmuth opened this issue Dec 18, 2023 · 2 comments · Fixed by #30162
Closed
Tracked by #28892

[pkg/ottl] Validate that all parts of a Key are used by a Context #30051

TylerHelmuth opened this issue Dec 18, 2023 · 2 comments · Fixed by #30162
Labels
enhancement New feature or request pkg/ottl priority:p2 Medium

Comments

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Dec 18, 2023 •
edited
Loading

Component(s)

pkg/ottl

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

OTTL currently validates that a context "consumes" all segments in a path, but does not validate that all a path's keys are used. This means users can do something like name["foo"] in the span context and get back the name of the span, completely ignoring the foo key. In this scenario the context is allowing parsing to succeed when it shouldnt. To simplify this process for all the different contexts, we can build something into OTTL, similar to Path validation, but for Keys.

Things to consider:

  • Should we validate just the first key? - [pkg/ottl] Don't check that all keys are used #30047 (comment)
  • How will key validation interact with dynamic indexing? If we don't know the value of the key until hot path execution, how should we validate that all keys are being considered?
  • Should we have a way for contexts to declare that they've reached a point where the map is arbitrarily deep? So a context exposing a path of underlying type map[string]string would not declare this since stringMap["key"]["nestedKey"] would be invalid. But a context would declare this for an attributes path since the type is map[string]any and attributes["key1"]["key2"][...] is valid. - Originally from [pkg/ottl] Don't check that all keys are used #30047 (comment)
@TylerHelmuth TylerHelmuth added enhancement New feature or request needs triage New item requiring triage pkg/ottl priority:p2 Medium and removed needs triage New item requiring triage labels Dec 18, 2023
This was referenced Dec 18, 2023
Open
Merged
TylerHelmuth added a commit that referenced this issue Dec 18, 2023
@TylerHelmuth
[pkg/ottl] Don't check that all keys are used (#30047)
f9807d0 
**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
#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:** 

#22744

#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
#28642.
@github-actions github-actions bot mentioned this issue Dec 19, 2023
Closed
@TylerHelmuth
Copy link
Member Author

@evan-bradley I though about this some more and I think there is a clean solution if we go back to keys being represented by a slice instead of a linked list.

Unlike the path segments, the keys are used during hot-path execution which means the next concept doesn't fit them as well. If a context needs keys, it needs all of them so it can feed them into the Map/Slice functions (or something else).

I think we can solve this problem by having OTTL record whether the context asked for all the keys or not. If a context has a requirement to only allow a specific number of keys, such as ottlspan's trace_state, then the context can be responsible for counting the number of keys. If a context doesn't request the keys for a given path, we'll be able to fail. This means situations like replace_match(body["metadata"]["uid"], "*", "12345") will be allowed but name["invalid"] will fail.

@TylerHelmuth
Copy link
Member Author

Which is also pretty annoying because ottl.Key interface ends up really similar to the previous struct from the grammar and now I have to change a bunch of tests again lol

@TylerHelmuth TylerHelmuth mentioned this issue Dec 21, 2023
Merged
TylerHelmuth added a commit that referenced this issue Jan 8, 2024
@TylerHelmuth @evan-bradley
[pkg/ottl] Validate Keys are used (#30162)
a4853ed 
**Description:** 
This PR updates OTTL's validation logic to validate the keys are
considered during path parsing.

Unlike the path, keys are needed during hot path execution. For that
reason, the contexts need all the keys during parsing, not an individual
key that might be linked to the next key.

This PR updates the Path interface to have a list of keys instead of a
link to the first Key. This keeps the keys interaction closer to the
original key struct from the grammar, which means when the interfaces
release we have less breaking changes.

**Link to tracking Issue:** <Issue number if applicable>
Closes
#30051

**Testing:** <Describe what testing was performed and which tests were
added.>
Added unit tests to validate that indexing fails on paths that dont use
keys and pass on paths that allow keys.

---------

Co-authored-by: Evan Bradley <[email protected]>
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jan 10, 2024
@TylerHelmuth @cparkins
[pkg/ottl] Don't check that all keys are used (open-telemetry#30047)
fbdc8ee 
**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.
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jan 10, 2024
@TylerHelmuth @evan-bradley @cparkins
[pkg/ottl] Validate Keys are used (open-telemetry#30162)
28bae3e 
**Description:** 
This PR updates OTTL's validation logic to validate the keys are
considered during path parsing.

Unlike the path, keys are needed during hot path execution. For that
reason, the contexts need all the keys during parsing, not an individual
key that might be linked to the next key.

This PR updates the Path interface to have a list of keys instead of a
link to the first Key. This keeps the keys interaction closer to the
original key struct from the grammar, which means when the interfaces
release we have less breaking changes.

**Link to tracking Issue:** <Issue number if applicable>
Closes
open-telemetry#30051

**Testing:** <Describe what testing was performed and which tests were
added.>
Added unit tests to validate that indexing fails on paths that dont use
keys and pass on paths that allow keys.

---------

Co-authored-by: Evan Bradley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request pkg/ottl priority:p2 Medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[pkg/ottl] Validate Keys are used TylerHelmuth/opentelemetry-collector-contrib
1 participant
@TylerHelmuth