tracing.opentracing endpoint middleware options#1072
tracing.opentracing endpoint middleware options#1072basvanbeek merged 9 commits intogo-kit:masterfrom
Conversation
- optionally trace business errors - add option to set static/dynamic tags - add option to set dynamic name
- some renaming - more flexible tags option: support adding tags with multiple options
- TraceServer and TraceClient should differ only in span kind tag value - remain TraceServer and TraceClient for backward compatibility
|
Guys, Is it any chance to get it reviewed? |
|
Hi @alebabai, thanks for the PR and sorry for the late action. I will review tomorrow. |
basvanbeek
left a comment
basvanbeek
left a comment
There was a problem hiding this comment.
Great effort. Only a few nits mostly with respect to having more symmetry with the OpenCensus middleware.
tracing/opentracing/endpoint.go
Outdated
| // OpenTracing Span called `operationName` with client span.kind tag. | ||
| func TraceClient(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { | ||
| opts = append(opts, WithTags(map[string]interface{}{ | ||
| ext.SpanKindRPCServer.Key: ext.SpanKindRPCClient.Value, |
There was a problem hiding this comment.
| ext.SpanKindRPCServer.Key: ext.SpanKindRPCClient.Value, | |
| ext.SpanKindRPCClient.Key: ext.SpanKindRPCClient.Value, |
tracing/opentracing/endpoint.go
Outdated
| ctx = opentracing.ContextWithSpan(ctx, span) | ||
|
|
||
| response, err := next(ctx, request) | ||
| if err := identifyError(response, err, cfg.IgnoreBusinessError); err != nil { |
There was a problem hiding this comment.
Would like more symmetry with the OpenCensus implementation and also include the handling of Go kit load balancer retry errors.
There was a problem hiding this comment.
it would also be nice that even if an ignore business error is set to true, we would still see it being tagged with "gokit.business.error", again in-line with the OpenCensus implementation.
There was a problem hiding this comment.
Yep, you're right.
Implemented it in way as it was done in opencensus, now it support all the stuff.
- support lb.Retry errors - add more logging fields as it done in opencensus - log business error anyway - improve codebase - fix typo - add more test cases
alebabai
force-pushed
the
feature-tracing-opentracing
branch
from
5cab590 to
e5c83b1
Compare
basvanbeek
left a comment
basvanbeek
left a comment
There was a problem hiding this comment.
@alebabai thank you so much for hanging in there and having the patience to wait for me to finish the review. The code is now finally inline with functionality as found in the other tracers so I'm grateful for the contribution!
4 participants
what
TraceEndpointfunction to not rely on client/server kinds.tracing.opentracingendpoint middleware API with some options (like it was done for Opencensus):ext.LogErrorfunction)why
closes #804