Skip to content
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

proposal: ddtrace/tracer: check for nil pointer in interface. #2029

Open
LevOlkha opened this issue Jun 8, 2023 · 2 comments · May be fixed by #2036
Open

proposal: ddtrace/tracer: check for nil pointer in interface. #2029

LevOlkha opened this issue Jun 8, 2023 · 2 comments · May be fixed by #2036
Labels
ack needs-investigation requires work from team member proposal more in depth change that requires full team approval tracer

Comments

@LevOlkha
Copy link

LevOlkha commented Jun 8, 2023

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.):

@LevOlkha LevOlkha added the bug unintended behavior that has to be fixed label Jun 8, 2023
@LevOlkha LevOlkha changed the title [BUG] [BUG] nil interface for error misinterpreted Jun 8, 2023
@ajgajg1134 ajgajg1134 added the ack label Jun 12, 2023
@knusbaum knusbaum changed the title [BUG] nil interface for error misinterpreted proposal: ddtrace/tracer: check for nil pointer in interface. Aug 10, 2023
@knusbaum knusbaum added needs-investigation requires work from team member tracer proposal more in depth change that requires full team approval and removed bug unintended behavior that has to be fixed labels Aug 10, 2023
@knusbaum
Copy link
Contributor

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:

@LevOlkha
Copy link
Author

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: #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: #2036 does not show any performance regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack needs-investigation requires work from team member proposal more in depth change that requires full team approval tracer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants