[pkg/ottl] Add contains function #33078
Conversation
lkwronski
requested review from
TylerHelmuth,
bogdandrutu and
evan-bradley
as code owners
pkg/ottl/ottlfuncs/func_contains.go
Outdated
| ) | ||
|
|
||
| type ContainsArguments[K any] struct { | ||
| Target []ottl.StringLikeGetter[K] |
There was a problem hiding this comment.
this won't allow you to pass a slice, consider this use case
contains(attributes["tags"], "staging")
There was a problem hiding this comment.
Maybe I'm wrong, but I tried to write e2e test and it seems that the following statement works and I can pass the slices:
set(attributes["test"], "pass") where Contains(["hello", "world"], "hello")
Did I miss anything?
There was a problem hiding this comment.
the thing is you're just assuming StringLikeGetter,
when I want to use config like this
- context: log
statements:
- set(attributes["tags"], ["staging", "hello", "world", "work"])
- set(attributes["staging"], "true") where Contains(attributes["tags"], "staging")Contains fail because first attribute is not an array.
so i need to fix it like this
log_statements:
- context: log
statements:
- set(attributes["tags"], ["staging", "hello", "world", "work"])
- set(attributes["staging"], "true") where Contains([attributes["tags"]], "staging")by wrapping attributes["tags"] with [] to make it an array.
when i do that, your StringLikeGetter will get the attributes["tags"] and finds out it's a Slice. So it makes a string out of slice by marshaling the array.
Now you have marshaled array as a string["staging", "hello", "world", "work"] that does not equal staging in my condition.
What you need to do is to use Getter and work with different types you may get. You may get Slice, Map or a string wrapped in pcommon.Value. You should handle all of these differently.
While you're at it changing behavior to match what i described i would be the same way to add support for ints, floats and boolean as you were asking in the other comment
It will also fix UX.
I want to use Contains(["a", "b"...] and Contains(attributes["tags"] at the same time without wrapping workaround
There was a problem hiding this comment.
I really appreciate the detailed answer that helps me understand the flow. Thanks.
I added e2e with the scenario you described. I hope it is better now.
lkwronski
force-pushed
the
lkwronski.issue-30420-contains
branch
2 times, most recently
from
a43631c
to
4ef84a9
Compare
lkwronski
force-pushed
the
lkwronski.issue-30420-contains
branch
from
4ef84a9
to
2809a2f
Compare
pkg/ottl/expression.go
Outdated
| } | ||
|
|
||
| // converts a generic slice to pcommon.Slice. It uses reflection to handle any slice type, creating a pcommon.Slice from it. | ||
| func convertToSlice(val any) (pcommon.Slice, error) { |
There was a problem hiding this comment.
not sure i like that, especially when we have so few types we support
There was a problem hiding this comment.
You're right, I removed the reflection and it simplified things a bit.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@evan-bradley Is there anything else to do in this PR? |
|
@lkwronski I'm sorry, I lost track of this. It looks good to me. @TylerHelmuth Please take a look. |
| case strings.HasPrefix(name, "PSliceGetter"): | ||
| arg, err := p.newGetter(argVal) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return StandardPSliceGetter[K]{Getter: arg.Get}, nil |
There was a problem hiding this comment.
Adding the new Getter requires some new tests in functions_test.go that hit this case
| "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" | ||
| ) | ||
|
|
||
| func Test_contains(t *testing.T) { |
There was a problem hiding this comment.
Can you add a test for the item being a pcommon.Value, pcommon.Map, and pcommon.Slice. I suspect those cases are not going to work bc we take the item as is.
There was a problem hiding this comment.
Do you mean that such a case should work? lkwronski@417027d#diff-d1b51ac42b51c96343723ba6724c7ba49e3880a1e35e9ba6557711fb34d266c5R142
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
| if val == item { | ||
| return true, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Although functionally correct and generic enough to cover multiple data types, this approach can become prohibitively expensive for performance sensitive applications under load. For example, if this approach was used for supporting routing-connector in open-telemetry collector to inspect 100+ string values while processing 100,000 spans per second kind of workload. A map would be much more efficient for such kind of conditions.
There was a problem hiding this comment.
just to clarify, when i say multiple data types.... I meant to appreciate that it can handle integer, strings, booleans alike.... However I am concerned that outer type has to be a slice only... which may not be the optimal choice for heavy workloads of say, simple string based look up.
There was a problem hiding this comment.
I understand your concern, but I'm not sure how to do it with a map. If we want to handle Contains that accept a slice as input, I don't see how to convert it to a map in linear time.
If we accepted map as input in Contains, it would be easy to get an efficient search, but I'm not sure how to do it with slice.
| }, | ||
| item: ottl.StandardGetSetter[any]{ | ||
| Getter: func(_ context.Context, _ any) (any, error) { | ||
| return "unknow", nil |
There was a problem hiding this comment.
not sure if it was intentional to make this a typo error... you probably meant to write unknown
|
Looking forward to become this function "alive". I also have a use case for this |
|
Any news on this function @lkwronski ? I'm really looking forward to using it |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Any news on this? This function will be very handy. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Sorry for the long response time, but I finally have time to finalize this PR. @TylerHelmuth Can you re-open this PR? I have already pushed commit to my branch, but it isn't visible here due to closed PR lkwronski@417027d. |
Description: Adds a new contains function to check if slice contains item.
Link to tracking Issue: Fixes #30420
Testing: Added unit test
Documentation: TODO