-
Notifications
You must be signed in to change notification settings - Fork 435
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
ddtrace/opentracer: translate Datadog errors to OpenTracing errors #1009
Conversation
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.
Looks mostly good.
The tests are good, but it would be really nice to ensure that Inject
and Extract
are actually returning them.
ddtrace/opentracer/tracer_test.go
Outdated
func TestTranslateError(t *testing.T) { | ||
t.Run("nil", func(t *testing.T) { | ||
assert.Nil(t, translateError(nil)) | ||
}) | ||
t.Run("unrecognized", func(t *testing.T) { | ||
err := errors.New("unrecognized") | ||
assert.Equal(t, err, translateError(err)) | ||
}) | ||
t.Run("ErrSpanContextNotFound", func(t *testing.T) { | ||
assert.Equal(t, opentracing.ErrSpanContextNotFound, translateError(tracer.ErrSpanContextNotFound)) | ||
}) | ||
t.Run("ErrInvalidCarrier", func(t *testing.T) { | ||
assert.Equal(t, opentracing.ErrInvalidCarrier, translateError(tracer.ErrInvalidCarrier)) | ||
}) | ||
t.Run("ErrInvalidSpanContext", func(t *testing.T) { | ||
assert.Equal(t, opentracing.ErrInvalidSpanContext, translateError(tracer.ErrInvalidSpanContext)) | ||
}) | ||
t.Run("ErrSpanContextCorrupted", func(t *testing.T) { | ||
assert.Equal(t, opentracing.ErrSpanContextCorrupted, translateError(tracer.ErrSpanContextCorrupted)) | ||
}) | ||
} |
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 think this would be better as a table test (example).
That will reduce the boilerplate and make it simpler to update if we ever add more errors.
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.
Refactored tests to be table tests and test that Inject
and Extract
actually return the correct error.
This change fixes so that the OpenTracing implementation returns standard errors defined in opentracing-go, as opposed to returning errors defined in ddtrace/tracer. Fixes DataDog#998
e1b14d5
to
e2c2612
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.
Beautiful. Thank you, @krosen040
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.
Actually, just noticed one more thing.
Tests seem to be missing ErrInvalidSpanContext
. That should be a small addition to what you have to completely cover the errors.
See here for the logic that produces an ErrInvalidSpanContext
(looks like just spanid = 0 || traceid == 0 || ctx.(type) != *tracer.spanContext
)
dd-trace-go/ddtrace/tracer/textmap.go
Lines 217 to 221 in d62beaa
func (p *propagator) injectTextMap(spanCtx ddtrace.SpanContext, writer TextMapWriter) error { | |
ctx, ok := spanCtx.(*spanContext) | |
if !ok || ctx.traceID == 0 || ctx.spanID == 0 { | |
return ErrInvalidSpanContext | |
} |
And here for the tests that generate it. We only need to do one to ensure the conversion.
dd-trace-go/ddtrace/tracer/textmap_test.go
Lines 91 to 96 in d62beaa
err = propagator.Inject(internal.NoopSpanContext{}, TextMapCarrier(map[string]string{})) | |
assert.Equal(ErrInvalidSpanContext, err) | |
err = propagator.Inject(&spanContext{}, TextMapCarrier(map[string]string{})) | |
assert.Equal(ErrInvalidSpanContext, err) // no traceID and spanID | |
err = propagator.Inject(&spanContext{traceID: 1}, TextMapCarrier(map[string]string{})) | |
assert.Equal(ErrInvalidSpanContext, err) // no spanID |
…s translated correctly
Added a test for |
This change fixes so that the OpenTracing implementation returns standard errors
defined in opentracing-go, as opposed to returning errors defined in ddtrace/tracer.
Fixes #998