[jaeger-client-java/jax-rs2] Remove jaeger-context dependence for JAX-RS2 jaeger instrumentation #319

dray92 · 2018-01-20T23:28:41Z

Have not removed from build.gradle since deprecated class TracingUtils still depends on it.

codecov · 2018-01-20T23:34:45Z

Codecov Report

Merging #319 into master will increase coverage by 0.38%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #319      +/-   ##
============================================
+ Coverage     83.79%   84.17%   +0.38%     
- Complexity      563      565       +2     
============================================
  Files            91       91              
  Lines          2222     2225       +3     
  Branches        257      258       +1     
============================================
+ Hits           1862     1873      +11     
+ Misses          258      251       -7     
+ Partials        102      101       -1

Impacted Files	Coverage Δ	Complexity Δ
...a/com/uber/jaeger/filters/jaxrs2/TracingUtils.java	`42.85% <ø> (-14.29%)`	`2 <0> (-1)`
...a/com/uber/jaeger/filters/jaxrs2/ServerFilter.java	`95% <ø> (ø)`	`8 <0> (ø)`	⬇️
...eger/filters/jaxrs2/ClientSpanInjectionFilter.java	`84.21% <ø> (+10.52%)`	`5 <0> (+1)`	⬆️
...a/com/uber/jaeger/filters/jaxrs2/ClientFilter.java	`100% <ø> (ø)`	`5 <0> (ø)`	⬇️
...aeger/filters/jaxrs2/ClientSpanCreationFilter.java	`100% <100%> (ø)`	`4 <2> (+1)`	⬆️
...ger-core/src/main/java/com/uber/jaeger/Tracer.java	`86.89% <0%> (+0.43%)`	`20% <0%> (+1%)`	⬆️
...java/com/uber/jaeger/reporters/RemoteReporter.java	`90% <0%> (+2.85%)`	`9% <0%> (ø)`	⬇️
...jaeger/reporters/protocols/ThriftUdpTransport.java	`84.74% <0%> (+3.38%)`	`14% <0%> (ø)`	⬇️
...ain/java/com/uber/jaeger/senders/ThriftSender.java	`77.55% <0%> (+4.08%)`	`10% <0%> (ø)`	⬇️

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 0a5090e...1751422. Read the comment docs.

yurishkuro · 2018-01-21T00:07:17Z

Related to #188

yurishkuro · 2018-01-21T00:08:19Z

jaeger-jaxrs2/src/test/java/com/uber/jaeger/filters/jaxrs2/ClientFilterTest.java

@@ -63,8 +60,7 @@ public void setUp() throws Exception {
    tracer =
        new com.uber.jaeger.Tracer.Builder("Angry Machine", reporter, new ConstSampler(true))
            .build();
-    traceContext = new ScopeManagerTraceContext(tracer.scopeManager());
-    undertest = new ClientFilter(tracer, traceContext);
+    undertest = new ClientFilter(tracer);


Doing this reduces code coverage because the deprecated methods are no longer exercised. Is there a way to exclude them from coverage?

let me look around

jacoco plugin version 0.8 seems to support this {PR}

Methods annotated with @lombok.Generated (generated by Lombok getters, setters, equals, hashcode, toString, etc)

yeah, that looks dirty. Why not just invoke the deprecated methods here, like new ClientFilter(tracer, null);? Strictly speaking just because a function is deprecated doesn't mean it does not need to work correctly, so exercising it in tests is a good thing.

haha yes looks very wretched

yup! just gonna call the deprecate methods :)

dray92 · 2018-01-21T08:43:39Z

@yurishkuro lombok generated annotation + jacoco 0.8 definitely seems to do the trick, but it is a little bit of a hack - depends on how you feel about it :)

vprithvi

Looks good, also remove the jaeger-context subproject dependency in build.gradle

vprithvi · 2018-01-22T16:44:21Z

build.gradle

@@ -46,6 +46,10 @@ allprojects {
    }
 }

+jacoco {
+    toolVersion = "0.8.0"


Is this still required?

nah - not really; getting rid of it

dray92 · 2018-01-22T17:39:12Z

@vprithvi Can't get rid of jaeger-context from jaeger-jaxrs2/build.gradle

Deprecated methods will need it as compile time dep :(
Leaving a comment/fixme item for when the deprecated functions are removed

yurishkuro · 2018-01-23T20:18:51Z

jaeger-jaxrs2/src/test/java/com/uber/jaeger/filters/jaxrs2/ServerFilterTest.java

@@ -60,7 +57,7 @@ public void setUp() {
    tracer =
        new com.uber.jaeger.Tracer.Builder("Angry Machine", reporter, new ConstSampler(true))
            .build();
-    undertest = new ServerFilter(tracer);
+    undertest = new ServerFilter(tracer, null);


may want to add a comment "Using deprecated constructor for test coverage"

yurishkuro · 2018-01-23T20:19:08Z

jaeger-jaxrs2/src/test/java/com/uber/jaeger/filters/jaxrs2/ClientFilterTest.java

@@ -63,8 +60,7 @@ public void setUp() throws Exception {
    tracer =
        new com.uber.jaeger.Tracer.Builder("Angry Machine", reporter, new ConstSampler(true))
            .build();
-    traceContext = new ScopeManagerTraceContext(tracer.scopeManager());
-    undertest = new ClientFilter(tracer, traceContext);
+    undertest = new ClientFilter(tracer, null);


may want to add a comment "Using deprecated constructor for test coverage"

yurishkuro · 2018-01-23T20:20:55Z

jaeger-jaxrs2/src/test/java/com/uber/jaeger/filters/jaxrs2/ClientSpanInjectionFilterTest.java

+    tracer.scopeManager().activate(currentSpan, true);
+    when(clientRequestContext.getProperty(CURRENT_SPAN_CONTEXT_KEY)).thenReturn(currentSpan);
+
+    // FIXME (debo): remove to use {@link ClientSpanInjectionFilter(Tracer)}


may want to add a comment "Using deprecated constructor for test coverage"

…-RS2 tracing instrumentation Signed-off-by: Debosmit Ray <debo@uber.com>

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

…-RS2 tracing instrumentation Signed-off-by: Debosmit Ray <debo@uber.com>

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

dray92 · 2018-01-26T07:53:52Z

rebased on master

debo-C02V61KPHTDG:jaeger-client-java Debo$ git diff origin/jaxrs2_remove_tracing_utils
diff --git a/.travis.yml b/.travis.yml
index 74e0fb1..6524764 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -37,7 +37,7 @@ before_install:
 - ./travis/prepare-signing.sh $encrypted_677f232983c0_key $encrypted_677f232983c0_iv
 - if [ "$CROSSDOCK" == true ]; then bash ./travis/install-crossdock-deps.sh ; fi
 script:
-- if [ "$TESTS" == true ]; then make test ; else echo 'skipping tests'; fi
+- if [ "$TESTS" == true ]; then travis_wait 25 make test-travis ; else echo 'skipping tests'; fi
 - if [ "$COVERAGE" == true ]; then ./gradlew codeCoverageReport ; else echo 'skipping coverage'; fi
 - if [ "$CROSSDOCK" == true ]; then bash ./travis/build-crossdock.sh ; else echo 'skipping crossdock'; fi
 after_success:
diff --git a/Makefile b/Makefile
index 81d38f6..7170c69 100644
--- a/Makefile
+++ b/Makefile
@@ -8,6 +8,10 @@ clean:
 test:
        ./gradlew check
 
+.PHONY: test-travis
+test-travis:
+       ./gradlew -is check --info
+
 .PHONY: release
 release: clean
        ./gradlew -i uploadArchives

yurishkuro · 2018-01-30T00:06:15Z

can you rebase off master?

yurishkuro · 2018-01-30T00:06:58Z

nvm, I think I was able to do it myself

yurishkuro approved these changes Jan 21, 2018

View reviewed changes

yurishkuro reviewed Jan 21, 2018

View reviewed changes

vprithvi reviewed Jan 22, 2018

View reviewed changes

yurishkuro approved these changes Jan 23, 2018

View reviewed changes

dray92 force-pushed the jaxrs2_remove_tracing_utils branch 2 times, most recently from f2e5f18 to 4edc8cd Compare January 24, 2018 06:15

debosmit added 18 commits January 25, 2018 21:27

[jaeger-client-java/jax-rs2] Remove jaeger-context dependence for JAX…

76e008b

…-RS2 tracing instrumentation Signed-off-by: Debosmit Ray <debo@uber.com>

fix some docstrings + add lombok Generated annotation

0d4684c

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

try to pull in version 0.8 of jacoco

04fe396

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

add comment string about lombok.generated annotation

930bd71

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

remove lombok generated

4cd12a9

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

change tests to exercise deprecated methods

c18607c

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

resolve checkstyle issues.......

af55e2a

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

more docstring stuff.....

58a8b3f

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

[jaeger-client-java/jax-rs2] Remove jaeger-context dependence for JAX…

e1caa77

…-RS2 tracing instrumentation Signed-off-by: Debosmit Ray <debo@uber.com>

fix some docstrings + add lombok Generated annotation

3fd50b0

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

try to pull in version 0.8 of jacoco

7aea594

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

add comment string about lombok.generated annotation

ed6ce0d

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

remove lombok generated

9e8bf8c

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

change tests to exercise deprecated methods

43b0ae3

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

resolve checkstyle issues.......

df184a8

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

more docstring stuff.....

6032a7a

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

jaeger jersey instrumentation respects tracer's current active span

cdcf251

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

leaving comment re: deprecated constructor usage

7c02f90

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

dray92 force-pushed the jaxrs2_remove_tracing_utils branch from 4edc8cd to 7c02f90 Compare January 26, 2018 07:52

Merge branch 'master' into jaxrs2_remove_tracing_utils

51fc5eb

ghost assigned yurishkuro Jan 30, 2018

ghost added the review label Jan 30, 2018

Merge branch 'master' into jaxrs2_remove_tracing_utils

1751422

vprithvi approved these changes Jan 30, 2018

View reviewed changes

vprithvi merged commit 0b1b547 into jaegertracing:master Jan 30, 2018

ghost removed the review label Jan 30, 2018

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jaeger-client-java/jax-rs2] Remove jaeger-context dependence for JAX-RS2 jaeger instrumentation #319

[jaeger-client-java/jax-rs2] Remove jaeger-context dependence for JAX-RS2 jaeger instrumentation #319

dray92 commented Jan 20, 2018

codecov bot commented Jan 20, 2018 •

edited

yurishkuro commented Jan 21, 2018

yurishkuro Jan 21, 2018

dray92 Jan 21, 2018

dray92 Jan 21, 2018 •

edited

yurishkuro Jan 21, 2018

dray92 Jan 21, 2018

dray92 commented Jan 21, 2018

vprithvi left a comment

vprithvi Jan 22, 2018

dray92 Jan 22, 2018

dray92 commented Jan 22, 2018

yurishkuro Jan 23, 2018

yurishkuro Jan 23, 2018

yurishkuro Jan 23, 2018

dray92 commented Jan 26, 2018

yurishkuro commented Jan 30, 2018

yurishkuro commented Jan 30, 2018

[jaeger-client-java/jax-rs2] Remove jaeger-context dependence for JAX-RS2 jaeger instrumentation #319

[jaeger-client-java/jax-rs2] Remove jaeger-context dependence for JAX-RS2 jaeger instrumentation #319

Conversation

dray92 commented Jan 20, 2018

codecov bot commented Jan 20, 2018 • edited

Codecov Report

yurishkuro commented Jan 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dray92 Jan 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dray92 commented Jan 21, 2018

vprithvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dray92 commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dray92 commented Jan 26, 2018

yurishkuro commented Jan 30, 2018

yurishkuro commented Jan 30, 2018

codecov bot commented Jan 20, 2018 •

edited

dray92 Jan 21, 2018 •

edited