Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Adding annotated filter priorities for Jersey filters #329

Merged
merged 2 commits into from
Feb 1, 2018

Conversation

dray92
Copy link
Contributor

@dray92 dray92 commented Jan 31, 2018

Some open bugs in Jersey (#2912, #3467) cause the framework to not honor filter binding priorities. For our internal libraries, we have found that keeping the binding priorities in the [5001, ∞) range while registering the filters keeps the ordering intact.

This change attempts to ensure that span is created very early in the request life-cycle, while giving room for consumers to manually register filters to priorities greater than javax.ws.rs.Priorities.USER, and yet, come before the span injection stage.

Similar idea follows for the server side instrumentation.

Signed-off-by: Debosmit Ray debo@uber.com

Signed-off-by: Debosmit Ray <debo@uber.com>
@codecov
Copy link

codecov bot commented Jan 31, 2018

Codecov Report

Merging #329 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #329      +/-   ##
============================================
- Coverage     84.04%   83.91%   -0.14%     
+ Complexity      567      565       -2     
============================================
  Files            91       91              
  Lines          2225     2225              
  Branches        258      258              
============================================
- Hits           1870     1867       -3     
- Misses          256      257       +1     
- Partials         99      101       +2
Impacted Files Coverage Δ Complexity Δ
...eger/filters/jaxrs2/ClientSpanInjectionFilter.java 84.21% <ø> (ø) 5 <0> (ø) ⬇️
...aeger/filters/jaxrs2/ClientSpanCreationFilter.java 100% <ø> (ø) 4 <0> (ø) ⬇️
...a/com/uber/jaeger/filters/jaxrs2/ServerFilter.java 95% <ø> (ø) 8 <0> (ø) ⬇️
...jaeger/reporters/protocols/ThriftUdpTransport.java 81.35% <0%> (-5.09%) 14% <0%> (-2%)

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 fda7ade...343ee5d. Read the comment docs.

@yurishkuro
Copy link
Member

hardcoding priorities seems like a bad thing. Can they not be passed to constructor or via some other mechanisms?

@ghost ghost assigned yurishkuro Feb 1, 2018
@ghost ghost added the review label Feb 1, 2018
@dray92
Copy link
Contributor Author

dray92 commented Feb 1, 2018

@yurishkuro So, I get your point. But, while registering, a user can specify priorities. This is what we are doing in our internal libraries now. To keep the filters and interceptors as self-contained units, it makes sense to give it sane defaults such as the ones we have here - and the @Priority annotation allows us to do just that. So, users can just plug in these components without having to worry about priorities.

@yurishkuro yurishkuro merged commit cc0d0f2 into jaegertracing:master Feb 1, 2018
@ghost ghost removed the review label Feb 1, 2018
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

3 participants