DataDog
proposal: ddtrace/tracer: check for nil pointer in interface.
Version of dd-trace-go
Describe what happened:
type MyError struct { msg string } func (e *MyError) Error() string { return e.msg } var err error var myError *MyError myError = nil err = myError span.Finish(tracer.WithError(err))
This is going to crash
the problem is in
https://github.com/DataDog/dd-trace-go/blob/v1.51.0/ddtrace/tracer/span.go#L427
instead of
if cfg.Error != nil
it should be something like
if !reflect.ValueOf(cfg.Error).IsNil()
Describe what you expected:
expected not to set error in the span. Steps to reproduce the issue:
Additional environment details (Version of Go, Operating System, etc.):
This requires some discussion as it's not clear exactly how we should handle this.
Reflection is possible, but there may be performance implications.
Go generally relies on users to make sure any value put in an interface can have the methods of that interface called on it.
That is, if e != nil and e is of type error, we should be able to call Error() on it. This is a somewhat controversial language choice, but one that is part of Go itself.
This is part of why you can't create non-nil error values that contain nil pointers. Notice the lack of error interface implementors in the errors package: https://pkg.go.dev/errors
Also, this can be worked around with the following:
func (e *MyError) Error() string {
if e == nil {
return ""
}
return e.msg
}
Ultimately we want it to be easy to use this and hard to panic, so I'm ok working around it, but we need to consider the implications.
See also:
- https://groups.google.com/g/golang-nuts/c/wnH302gBa4I
- https://go.dev/tour/methods/12
The workaround you propose will not work when we have a chain of functions that call some 3rd party library and return an error and tracer.WithError(err) is executed in the caller. It would require us to implement the check that was proposed in this PR: https://github.com/DataDog/dd-trace-go/pull/2036. This is what we currently do in our code as well. It seems logical to have this as a standard functionality, especially since the following scenario is already supported:
var err ... tracer.WithError(err)
Having a crash in one scenario and not in the other makes it significantly less intuitive, especially since this PR: https://github.com/DataDog/dd-trace-go/pull/2036 does not show any performance regression.
