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

Add proper support for a single RequestTracingFilter that supports Servlet 2.x and Servlet 3 environments at the same time #52

Merged
merged 1 commit into from
Oct 17, 2017

Conversation

nicmunroe
Copy link
Member

Fixes issue #50. Big thanks to @adriancole and @woldie for the help on this.

@codecov-io
Copy link

codecov-io commented Oct 16, 2017

Codecov Report

Merging #52 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #52      +/-   ##
============================================
+ Coverage     98.93%   98.94%   +<.01%     
+ Complexity      475      471       -4     
============================================
  Files            28       28              
  Lines          1132     1139       +7     
  Branches        134      133       -1     
============================================
+ Hits           1120     1127       +7     
  Misses            4        4              
  Partials          8        8
Impacted Files Coverage Δ Complexity Δ
...et/WingtipsRequestSpanCompletionAsyncListener.java 100% <ø> (ø) 8 <0> (ø) ⬇️
...ava/com/nike/wingtips/servlet/HttpSpanFactory.java 100% <ø> (ø) 10 <0> (?)
.../wingtips/servlet/RequestTracingFilterNoAsync.java 100% <ø> (ø) 2 <0> (ø) ⬇️
...tips/servlet/RequestWithHeadersServletAdapter.java 100% <ø> (ø) 4 <0> (?)
...om/nike/wingtips/servlet/RequestTracingFilter.java 100% <100%> (ø) 23 <22> (+13) ⬆️
...java/com/nike/wingtips/servlet/ServletRuntime.java 100% <100%> (ø) 3 <3> (?)

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 cd95cc6...d2a4d70. Read the comment docs.

Copy link

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

Wonderful

public static void beforeClass() throws Exception {
try {
ServletRequest.class.getMethod("getAsyncContext");

Choose a reason for hiding this comment

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

Smart

this module might need to do this. Which Servlet API dependency you pull in depends on the type of Servlet environment
you want to support (Servlet 2.x or Servlet 3+). For example:

* Servlet 3 API dependency: [`javax.servlet:javax.servlet-api:$servlet3ApiVersion`](http://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22javax.servlet%22%20AND%20a%3A%22javax.servlet-api%22)

Choose a reason for hiding this comment

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

Variables intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I can see how it looks like a copy/paste error. I'll see if I can clear it up.

* @param servletRequestClass ServletRequest implementation class
* @return true if the given servletRequest implementation class supports the getAsyncContext() method,
* otherwise false
* <p>If you want to prevent this filter from executing on specific requests then you can override {@link

Choose a reason for hiding this comment

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

Neat.. good idea to document this

// Make sure we set the trace ID on the response header now before the response is committed (if we wait
// until after the filter chain then the response might already be committed, silently preventing us
// from setting the response header)
response.setHeader(TraceHeaders.TRACE_ID, newSpan.getTraceId());

Choose a reason for hiding this comment

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

Ps eventhough b3 doesnt usually do this, I noticed amazon do. For example returning back the sample result (as everything is in one header)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I still have issue #30 to change this to have the response header be something other than X-B3-TraceId, but having the trace ID somewhere in the response headers has been critically useful.

…rvlet 2.x and Servlet 3 environments at the same time
@nicmunroe nicmunroe force-pushed the feature/old-servlet-support branch 2 times, most recently from 5021065 to d2a4d70 Compare October 17, 2017 16:06
@nicmunroe nicmunroe merged commit 15a0ab8 into Nike-Inc:master Oct 17, 2017
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.

None yet

3 participants