datadog: Add tracer option to check error eligibility in the span #1438
Conversation
|
|
|
|
There was a problem hiding this comment.
Looks mostly good to me, will want @Quinn-With-Two-Ns's thoughts.
| // ErrCheckFn can be set to a custom function to determine if an error should be traced. | ||
| ErrCheckFn func(err error) bool |
There was a problem hiding this comment.
Fn not an abbreviation we usually use. How about:
| // ErrCheckFn can be set to a custom function to determine if an error should be traced. | |
| ErrCheckFn func(err error) bool | |
| // CheckError can be set to a custom function to determine if an error should be traced. | |
| CheckError func(error) bool |
Or maybe even better and more flexible:
| // ErrCheckFn can be set to a custom function to determine if an error should be traced. | |
| ErrCheckFn func(err error) bool | |
| // OnFinish sets finish options. If unset, this will use [tracer.WithError] if | |
| // [interceptor.TracerFinishSpanOptions.Error] is non-nil and not | |
| // [workflow.IsContinueAsNewError]. | |
| OnFinish func(*interceptor.TracerFinishSpanOptions) []tracer.FinishOption |
@Quinn-With-Two-Ns, preference?
There was a problem hiding this comment.
Updated the naming, will be waiting for the final decision regarding the more flexible option.
There was a problem hiding this comment.
+1 to using TracerFinishSpanOptions to allow us to add more fields in the future.
| t.Span.Finish(opts...) | ||
| } | ||
|
|
||
| func (t *tracerSpan) shouldErrBeTraced(err error) bool { |
There was a problem hiding this comment.
Can just inline this, no need for a separate method (but no biggie)
| err := options.Error | ||
| isErrEligible := err != nil && | ||
| !workflow.IsContinueAsNewError(err) && | ||
| t.CheckError != nil && t.CheckError(err) |
There was a problem hiding this comment.
| t.CheckError != nil && t.CheckError(err) | |
| (t.CheckError == nil || t.CheckError(err)) |
Otherwise no error will ever be tracked unless they set the option. Alternatively you could just default check error to be a default impl that checks for continue-as-new error and tell users that so if they override, they can choose whether continue-as-new is counted.
| if opts.OnFinish == nil { | ||
| opts.OnFinish = func(options *interceptor.TracerFinishSpanOptions) []tracer.FinishOption { | ||
| var finishOpts []tracer.FinishOption | ||
|
|
||
| if err := options.Error; err != nil && !workflow.IsContinueAsNewError(err) { | ||
| finishOpts = append(finishOpts, tracer.WithError(err)) | ||
| } | ||
|
|
||
| return finishOpts | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of having if-else statement in the Finish function, I've decided to set a default function in the constructor.
There was a problem hiding this comment.
👍 I think this is a good approach
| if opts.OnFinish == nil { | ||
| opts.OnFinish = func(options *interceptor.TracerFinishSpanOptions) []tracer.FinishOption { | ||
| var finishOpts []tracer.FinishOption | ||
|
|
||
| if err := options.Error; err != nil && !workflow.IsContinueAsNewError(err) { | ||
| finishOpts = append(finishOpts, tracer.WithError(err)) | ||
| } | ||
|
|
||
| return finishOpts | ||
| } | ||
| } |
There was a problem hiding this comment.
👍 I think this is a good approach
vaihtovirta
force-pushed
the
datadog-err-check-fn-option
branch
from
7a32eca to
e62646d
Compare
What was changed
Added an option to datadog tracer that allows to determine whether an error is eligible to be added to the DD span.
Similar functionality in gorm dd instrumentation: WithErrorCheck
Why?
Not all errors need to be reported to Datadog (intermediate retry error, validation error, etc).
Users can filter out such errors via specifying a custom function via the option.
Checklist
How was this tested:
I've added simple unit tests.
Any docs updates needed?
I didn't find docs related to temporal datadog tracer in the repository, but potentially this page can mention the new option: https://docs.temporal.io/dev-guide/go/observability#useful-resources