Skip to content
This repository has been archived by the owner on Jan 3, 2019. It is now read-only.

Refactors tracing handler to use HttpServerParser #52

Merged
merged 2 commits into from Dec 15, 2017

Conversation

hyleung
Copy link
Owner

@hyleung hyleung commented Dec 13, 2017

This commit introduces an implementaton of HttpServerParser and
refactors the server tracing handler such that span name customization
is done via the HttpServerParser instead of directly in the tracing
handler.

In order to make this work, needed to add a wrapper around the standard
Ratpack Response so that things like the PathBinding and original
ServerRequest are available to the HttpServerParser.

Closes #51

This commit introduces an implementaton of HttpServerParser and
refactors the server tracing handler such that span name customization
is done via the HttpServerParser instead of directly in the tracing
handler.

In order to make this work, needed to add a wrapper around the standard
Ratpack Response so that things like the PathBinding and original
ServerRequest are available to the HttpServerParser.

Closes #51
@codecov
Copy link

codecov bot commented Dec 13, 2017

Codecov Report

Merging #52 into master will decrease coverage by 0.07%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #52      +/-   ##
============================================
- Coverage      79.9%   79.82%   -0.08%     
- Complexity       33       36       +3     
============================================
  Files             6        7       +1     
  Lines           209      228      +19     
  Branches          7        7              
============================================
+ Hits            167      182      +15     
- Misses           35       37       +2     
- Partials          7        9       +2
Impacted Files Coverage Δ Complexity Δ
.../main/java/ratpack/zipkin/ServerTracingModule.java 86.11% <100%> (-0.2%) 6 <0> (-1)
...k/zipkin/internal/DefaultServerTracingHandler.java 96.29% <100%> (+0.84%) 2 <0> (ø) ⬇️
...ava/ratpack/zipkin/internal/ServerHttpAdapter.java 66.66% <100%> (-16.67%) 4 <1> (-1)
...tpack/zipkin/internal/RatpackHttpServerParser.java 80% <80%> (ø) 5 <5> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecdc3b6...0a79dbb. Read the comment docs.

@codefromthecrypt
Copy link
Contributor

Looks good. FYI I will be raising something soon for "http.template", which is a tag that is uniform across templated requests, though not guaranteed to be a URI template.

@hyleung
Copy link
Owner Author

hyleung commented Dec 14, 2017

@adriancole thanks for taking a look! I'm wondering if I can get rid of all the explicit Span start/finish stuff that's in DefaultServerTracingHandler. As far as I can tell, that should all be taken care of by the HttpServerHandler.

final Span span = handler.handleReceive(extractor, request);
final Tracer.SpanInScope scope = tracing.tracer().withSpanInScope(span);
ctx.getResponse().beforeSend(response -> {
ServerResponse serverResponse = new ServerResponseImpl(response, request, ctx.getPathBinding());
handler.handleSend(serverResponse, null, span);
span.finish();
scope.close();
});
span.start();


//create a scope so that we can @Inject a SpanCustomizer into
//handlers and add tags, annotations, etc.
final Tracer.SpanInScope scope = tracing.tracer().withSpanInScope(span);
Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't actually know whether this is "a thing" - i.e. wanting to be able @Inject a SpanCustomizer into a handler and add tags, etc. I guess it might be useful if it's something that you don't want to implement as a general "policy" using something like a custom HttpServerParser ¯\(ツ)

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe harder hitting comment could be.. "place the scan in scope so that downstream code, such as logging can see the ids"

Not needed anymore because Brave's HttpServerHandler takes care of all
this now.
@hyleung hyleung merged commit 072e4cc into master Dec 15, 2017
@hyleung hyleung deleted the refactor-to-server-parser branch December 15, 2017 05:03
ctx.getResponse().beforeSend(response -> {
span.name(spanNameProvider.spanName(request, Optional.ofNullable(ctx.getPathBinding())));
handler.handleSend(response, null, span);
span.finish();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you asked.. yes, removing explicit start/finish was a good choice!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants