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

Fix issue where warning message is generated each time a null referen… #487

Merged
merged 4 commits into from
Jul 12, 2018

Conversation

objectiser
Copy link
Contributor

…ce is passed

Signed-off-by: Gary Brown [email protected]

When a null reference was being added, it was creating a warning message that was intended to detect non-JaegerSpanContext implementations. The message was also misleading as it was referring to the type of the referenceType (string) parameter rather than the reference.

@ghost ghost assigned objectiser Jul 11, 2018
@ghost ghost added the review label Jul 11, 2018
@codecov
Copy link

codecov bot commented Jul 11, 2018

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #487      +/-   ##
============================================
+ Coverage      88.2%   88.21%   +0.01%     
  Complexity      498      498              
============================================
  Files            65       65              
  Lines          1857     1859       +2     
  Branches        241      242       +1     
============================================
+ Hits           1638     1640       +2     
  Misses          141      141              
  Partials         78       78
Impacted Files Coverage Δ Complexity Δ
...n/java/io/jaegertracing/internal/JaegerTracer.java 89.83% <100%> (+0.08%) 24 <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 f3f6fdd...9bb4318. Read the comment docs.

Copy link
Collaborator

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Nice catch

@objectiser
Copy link
Contributor Author

@jpkrohling Ok for me to merge once coverage is fixed?

.withSampler(new ConstSampler(true))
.build();

JaegerSpan jaegerSpan = tracer.buildSpan("foo").asChildOf(NoopSpan.INSTANCE.context()).start();
Copy link
Member

Choose a reason for hiding this comment

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

I think it more belongs to span test class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

actually in tracer test class there is testAsChildOfAcceptNull but I think it more belongs here even span builder is in tracer class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry unclear - do you mean move the test back to the JaegerTracerTest class?

Copy link
Member

Choose a reason for hiding this comment

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

No it's fine here, there is different test in tracer test class which is similar to this tests. I think that one also belongs here

@objectiser
Copy link
Contributor Author

@pavolloffay Can this be merged now?

@pavolloffay pavolloffay merged commit b9e0282 into jaegertracing:master Jul 12, 2018
@ghost ghost removed the review label Jul 12, 2018
@pavolloffay
Copy link
Member

@objectiser done

@objectiser objectiser deleted the addrefnull branch July 12, 2018 08:42
@objectiser
Copy link
Contributor Author

@pavolloffay thanks

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.

3 participants