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

tracing.opentracing endpoint middleware options #1072

Merged
merged 9 commits into from
May 7, 2021

Conversation

alebabai
Copy link
Contributor

@alebabai alebabai commented Mar 22, 2021

what

  • Add TraceEndpoint function to not rely on client/server kinds.
  • TraceServer and TraceClient should differ only in span kind tag value
  • Extend tracing.opentracing endpoint middleware API with some options (like it was done for Opencensus):
    • optionally trace business errors
    • add option to set static/dynamic tags
    • add option to set dynamic name based on context and previous name
    • bump opentracing lib (to have ext.LogError function)

why

closes #804

- 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
@peterbourgon
Copy link
Member

@basvanbeek

@alebabai
Copy link
Contributor Author

Guys, Is it any chance to get it reviewed?

@basvanbeek
Copy link
Member

Hi @alebabai, thanks for the PR and sorry for the late action. I will review tomorrow.

@basvanbeek basvanbeek self-requested a review April 12, 2021 14:12
Copy link
Member

@basvanbeek basvanbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great effort. Only a few nits mostly with respect to having more symmetry with the OpenCensus middleware.

// 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ext.SpanKindRPCServer.Key: ext.SpanKindRPCClient.Value,
ext.SpanKindRPCClient.Key: ext.SpanKindRPCClient.Value,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks

ctx = opentracing.ContextWithSpan(ctx, span)

response, err := next(ctx, request)
if err := identifyError(response, err, cfg.IgnoreBusinessError); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like more symmetry with the OpenCensus implementation and also include the handling of Go kit load balancer retry errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@basvanbeek basvanbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

@basvanbeek basvanbeek merged commit 52851ec into go-kit:master May 7, 2021
@sagikazarmark sagikazarmark added this to the v0.11.0 milestone Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add error tag to spans on OT endpoint middleware
4 participants