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

Implement TracerFactory #527

Merged
merged 2 commits into from
Aug 31, 2018

Conversation

jpkrohling
Copy link
Collaborator

Signed-off-by: Juraci Paixão Kröhling [email protected]

Which problem is this PR solving?

Short description of the changes

  • Implements the new TracerFactory interface, changing the current tracer resolver to get a tracer from this new class (so that we have only one place knowing how to create a tracer).

@ghost ghost assigned jpkrohling Aug 21, 2018
@ghost ghost added the review label Aug 21, 2018
@codecov
Copy link

codecov bot commented Aug 21, 2018

Codecov Report

Merging #527 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #527      +/-   ##
============================================
+ Coverage     88.63%   88.64%   +0.01%     
- Complexity      515      517       +2     
============================================
  Files            66       67       +1     
  Lines          1891     1893       +2     
  Branches        243      243              
============================================
+ Hits           1676     1678       +2     
  Misses          141      141              
  Partials         74       74
Impacted Files Coverage Δ Complexity Δ
.../tracerresolver/internal/JaegerTracerResolver.java 100% <100%> (ø) 2 <1> (ø) ⬇️
...g/tracerresolver/internal/JaegerTracerFactory.java 100% <100%> (ø) 2 <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 8d325b9...6458d45. Read the comment docs.

@isaachier
Copy link
Contributor

I'm glad I ended up using JaegerObjectFactory instead of JaegerTracerFactory in #509. :)

@jpkrohling
Copy link
Collaborator Author

codecov is lying: it claims that JaegerTracerResolver#resolve isn't being called, which is false. Running the tests in IntelliJ with code coverage, shows that it gets hit once:

image

I'm adding an explicit call to that method anyway, but codecov is lying!

@yurishkuro
Copy link
Member

Afaik codecov just works off the coverage files we send to it. Worth checking if the execution is being captured there first.

@vprithvi
Copy link
Contributor

vprithvi commented Aug 22, 2018

@yurishkuro is correct here. You can inspect these files by clicking download on the codecov build view: https://codecov.io/gh/jaegertracing/jaeger-client-java/commit/c858eba8871b19269718525ab28b083e48446772/build

@jpkrohling Maybe I'm reading the coverage number wrong, but to me it seems like codecov shows that the line is covered

@jpkrohling
Copy link
Collaborator Author

It shows the line as covered now, as I'm making an explicit call to the method, but it wasn't before.

@jpkrohling
Copy link
Collaborator Author

I just tried locally with ./gradlew codeCoverageReport and there's something strange there...

image

If I have a code like this in the test:

    JaegerTracer tracer = ((JaegerTracerResolver) tracerResolver).resolve();
    assertNotNull(tracer);

I get a different coverage result:
image

I can't explain this, so, I'll just leave the test with the explicit call to make codecov happy. But it's still lying.

@isaachier
Copy link
Contributor

I happen to agree that I've had my fair share of issues with codecov. Don't know if it stems from the language tooling or the codecov analysis.

@yurishkuro
Copy link
Member

Still way better than coveralls

@jpkrohling
Copy link
Collaborator Author

Apart from the code coverage discussion, is this OK to get merged?

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling merged commit 8e9492f into jaegertracing:master Aug 31, 2018
@ghost ghost removed the review label Aug 31, 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.

4 participants