pkg/ottl: add context inference for expressions #40194
Conversation
github-actions
bot
requested review from
edmocosta,
evan-bradley and
TylerHelmuth
| @@ -367,7 +454,7 @@ func (pc *ParserCollection[R]) ParseStatements(statements StatementsGetter, opti | |||
| var inferredContext string | |||
| var err error | |||
| if len(conditionsValues) > 0 { | |||
| inferredContext, err = pc.contextInferrer.infer(statementsValues, conditionsValues) | |||
| inferredContext, err = pc.contextInferrer.infer(statementsValues, conditionsValues, nil) | |||
There was a problem hiding this comment.
I'm wondering if we need a WithContextInferenceValueExpressions to allow API users passing extra value expressions to the context inferrer. At the moment, we only have the transform processor using those extra options, where statements and reusable conditions are taken into consideration to infer the context, and they are being configured using different config keys.
I think soon or later we will need that, conditions and expressions for example are a good combination, and I can imagine components using OTTL value expressions as data selectors/getters, and conditions to filter/select data out.
There was a problem hiding this comment.
Hmm I can't think of why this would be needed, so I'm inclined to leave it out. Passing additional conditions for either a statement or a value expression makes sense to me, but I can't imagine why you would have out of band expressions.
There was a problem hiding this comment.
Please let me know how strongly you feel about adding that option, and if you have a use case in mind for it.
There was a problem hiding this comment.
Hmm I can't think of why this would be needed, so I'm inclined to leave it out. Passing additional conditions for either a statement or a value expression makes sense to me, but I can't imagine why you would have out of band expressions.
Value expressions could be used as data selectors by components, so instead of configuring plain field names, they could support value expressions and conditions at their configurations, and would still being able to use the same parser collection converter to parse both (similar to what the transform processor does here).
I just found out that the signaltometricsconnector is doing exactly that:
signaltometrics:
datapoints:
- name: gauge.to.exphistogram
conditions:
- metric.type == 1 AND resource.attributes["resource.foo"] != nil
exponential_histogram:
count: "1" # 1 count for each datapoint
value: Double(value_int) + value_doubleWith this new option, we could give the inferrer some tips by passing the value expression and conditions, so it would infer the datapoint context, and the callback converter would be able to work on both, conditions and value expressions.
The context inferred version of that config would possibly look like this:
signaltometrics:
- name: gauge.to.exphistogram
conditions:
- metric.type == 1 AND resource.attributes["resource.foo"] != nil
exponential_histogram:
count: "1" # 1 count for each datapoint
value: Double(datapoint.value_int) + datapoint.value_doubleThey could probably find alternative solutions for that (use the ParseValueExpression as starting point, for example), without having to add the new option and supporting it on all parse functions, but I think it would make this API a bit more flexible and easy to use, especially if one of those value expressions/conditions configs are not required, for example.
There was a problem hiding this comment.
Sorry, I'm not following. In that example, wouldn't you use ParseValueExpressions with WithContextInferenceConditions? I did add support for that already, per your other suggestion.
What I'm questioning is whether there would ever be a need for passing an option value expression. I can't think of any valid scenarios for that.
There was a problem hiding this comment.
Yes, as I mentioned using the ParseValueExpressions with WithContextInferenceConditions would be an alternative for that example. But what if I want use the ParseConditions as my entry point, not value expressions? maybe my conditions converter logic will apply some filtering, and optionally evaluate/parse the value expression depending on the conditions evaluation.
On the other hand, if I've some value expression, which depending on the result would apply some conditions, I would probably want to use the ParseValueExpressions as my entry point.
There was a problem hiding this comment.
Right, it depends on what the "primary" thing is. In the case of signaltometrics, it will definitely be the expression - the conditions are optional.
I can't think of a valid reason why you would have an optional expression accompanying a condition or statement. I'd rather not add something if it's never going to be used, but again, please do tell me if you feel strongly that it should be there and I will relent :)
There was a problem hiding this comment.
I think it would be especially useful for conditions, but adding that would imply handling that new option for statements as well, which probably won't be that useful.
Anyway, I don't think it's a must, and I'd be fine implementing it later when/if needed.
There was a problem hiding this comment.
👌 let's add it later as needed them. I'm happy to follow up.
Co-authored-by: Edmo Vamerlatti Costa <[email protected]>
There was a problem hiding this comment.
It looks good to me! Thanks!
@evan-bradley @TylerHelmuth, please review when you get a chance.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@evan-bradley @TylerHelmuth can you please take a look? |
@dmitryax this is an API-only change for now, not visible to users. Or are you saying there's some API docs that need updating? |
Description
Adds support for context inference when parsing OTTL value expressions. This is not used anywhere yet, but I intend to use it in a new processor that partitions batches of data based on OTTL expressions.
Link to tracking issue
Fixes #39158
Testing
Added/updated unit tests based on existing ones for ParseConditions.
Documentation
None