log: avoid panic marshaling nil error #131

Merged
merged 1 commit into from Dec 31, 2016

Projects

None yet

3 participants

@voutasaurus
Contributor

No description provided.

@bensigelman
Contributor

Thanks for fixing this. I'll give people 24h to comment and merge if there are no objections.

@yurishkuro
Contributor

A couple of questions:

  1. Why shouldn't it be the responsibility of the caller to check for err != nil before logging it to the span? To me, inserting an error field into the log implicitly says "there was an error".
  2. If we want to redefine the semantics of field.Error to be sensitive to nil, then maybe it should skip emitting the value altogether instead of writing a string "<nil>" (although this could cause other issues like some backend expecting the list of log fields to be non-empty).
@bensigelman
Contributor

@yurishkuro I am somewhat sympathetic to 2. but think we should make a best effort to prevent crashes/panics in production due to the monitoring code.

@voutasaurus
Contributor

This is an example where the assumption "LogFields won't panic" might hurt a production system:

if x, err := getX(); err != nil || invalid(x) {
    sp.LogFields(log.Error(err))
}

I'm open to not emitting anything on nil. As long as it doesn't panic I'll be happy.

@bensigelman bensigelman merged commit 5e5abf8 into opentracing:master Dec 31, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment