-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[processor/tailsampling] add OTTL Condition policy #20890
[processor/tailsampling] add OTTL Condition policy #20890
Conversation
…policy; added ctx to evaluate interface method
…ampling ottl policy
93386ed
to
063e3fc
Compare
…eval logic into different function
…when eval error happened
I will do the performance evaluation tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the README and add a changelog entry
…o ottl condition; added readme and changelog; removed unnecessary funcchain
ce47862
to
c89970a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good from an OTTL perspective. @jpkrohling something to consider for the future is that this policy option has overlapping capabilities with many other policy options. IDK how much flexibility there is to change this processor's config, but it could be beneficial to deprecate the duplicate policies.
/ptal conflict resolved @jpkrohling |
@TylerHelmuth , did you want create a follow up issue for this? Or should this be decided before this PR is merged? |
@MovieStoreGuy definitely doesn't need to hold up this PR. Only needs to be an issue if @jpkrohling wants to take the processor that direction. |
Alright I think I could solve the conflict if we are going to do the merge. So we don't need to trigger CI flow again and again. |
@jiekun I'd like the code owner @jpkrohling to review before we merge bc this is a significant change. Feel free to hold off handling any merge conflicts until he has reviewed. |
Type: OTTLCondition, | ||
OTTLConditionCfg: OTTLConditionCfg{ | ||
ErrorMode: ottl.IgnoreError, | ||
SpanConditions: []string{"attributes[\"test_attr_key_1\"] == \"test_attr_val_1\"", "attributes[\"test_attr_key_2\"] != \"test_attr_val_1\""}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: would single quotes work to wrap the attribute names (such as test_attr_key_1
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, OTTL requires double quotes ""
. In situations like these you could create the strings using back ticks ` instead of ".
} | ||
|
||
func (ocf *ottlConditionFilter) Evaluate(ctx context.Context, _ pcommon.TraceID, trace *TraceData) (Decision, error) { | ||
ocf.logger.Debug("Evaluating with OTTL conditions filter") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to make this message more useful? Like, by adding the trace ID to it? If the purpose is to give the user feedback that this works, you may want a metric counter instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to add a traceID
to it.
Actually the metric counter would be better. However, we have this debug log for all other tail sampling policies. I think it's bad when people see the log from other policies but empty output for ottl. They may wonder if it's not working.
And it's under Debug
log level which may help a very little bit. I think we should either replace all of them with a metric, or keep the log and add more information to make it useful. WDYT?
Line 41 in be4b4ef
c.logger.Debug("Evaluating spans counts in filter") |
|
||
// Span evaluation | ||
if ocf.sampleSpanExpr != nil { | ||
ocf.logger.Debug("Evaluating spans with OTTL conditions filter") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment. While this is useful for developing this component, extracting value from this log line is hard when you are operating the collector. Perhaps a counter would make sense here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed 2 unnecessary logging and keep the entry one only.
|
||
// Span event evaluation | ||
if ocf.sampleSpanEventExpr != nil { | ||
ocf.logger.Debug("Evaluating span events with OTTL conditions filter") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
}{ | ||
{ | ||
// policy | ||
"OTTL conditions not set", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an error condition? At least one of the conditions should be specified, I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I referred to the impl of filterprocessor:
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/be4b4efbb2dd32c9b1c7ded8720979fdeac33cd2/processor/filterprocessor/traces.go#LL45C3-L45C3
I think it's still good to stop user from using a blank condition(mistakenly). But I am not confident about the necessity to change it since we have validation at the begining of Evaluate
:
if ocf.sampleSpanExpr == nil && ocf.sampleSpanEventExpr == nil {
return NotSampled, nil
}
Feel free to let me know and I am ready to change the init function to return an error
when empty ottl condition is set.
Edit:
OK I prefer to check the condition during init. NewOTTLConditionFilter
is updated with check:
if len(spanConditions) == 0 && len(spanEventConditions) == 0 {
return nil, errors.New("expected at least one OTTL condition to filter on")
}
Test case updated as well, which expect an error
.
I'm fine merging this as is if the items I mentioned during my review are addressed in a follow-up PR. |
…aceID; Added inverse match test case; unexposed errorMode field
/ptal @jpkrohling Thanks for reviewing. I am lookging forward for more input since it's my first PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jiekun please resolve the conflicts then we'll be good to go.
/ptal @TylerHelmuth. Thanks for reviewing. I love how the OTel community responds to newbies' PR. I'm really looking forward to my 2nd contribution. |
) * [feature: OTTL sampling] Added OTTL config for tail-based sampling processor * [feature: OTTL sampling] Added OTTL impl struct * [feature: OTTL sampling] Change Query to Statement * [feature: OTTL sampling] Added bool evaluator for ottl tail sampling policy; added ctx to evaluate interface method * [feature: OTTL sampling] rename Eval response var * [feature: OTTL sampling] Added span and span event support for tail sampling ottl policy * [feature: OTTL sampling] Re-format the Evaluation method, move heavy eval logic into different function * [feature: OTTL sampling] Fixed typo * [feature: OTTL sampling] Simplified error log. return error decision when eval error happened * [feature: OTTL sampling] fixed typo and add debug log * [feature: OTTL sampling] Added config test for OTTL statement policy * [feature: OTTL sampling] Change after reivew: change ottl statement to ottl condition; added readme and changelog; removed unnecessary funcchain * [feature: OTTL sampling] make crosslink * [feature: OTTL sampling] commit tidy lint * Added license header to test file. * [feature: OTTL sampling] Change tail sampling processor sig to receive component otel settings instead of logger * [feature: OTTL sampling] Fixed lint issue * [feature: OTTL sampling] Fixed hardcoded errMode * [feature: unique trace path sampling] Review Changes: Logging with traceID; Added inverse match test case; unexposed errorMode field * [feature: OTTL tail sampling] Review Changes: Block empty OTTL condition config * [feature: ottl tail sampling policy] Sorting go.mod
Description: We could solve #20294 by introducing OTTL to the tail sampling processor. By using the new policy, the processor can make sampling decision based on the OTTL statement.
Link to tracking Issue: #20294
Testing: See
processor/tailsamplingprocessor/internal/sampling/ottl_test.go
.Documentation: See
README.me