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

Conversation

@quaff
Copy link
Contributor

@quaff quaff commented Mar 23, 2020

TextMapCodec.extract() return null with no warning if SPAN_CONTEXT_KEY not present, TraceContextCodec should align with it.
After switch TextMapCodec to TraceContextCodec, my application show lots of warning .
I have to change my code to suppress it.

SpanContext parent = tracer.extract(Format.Builtin.HTTP_HEADERS, new HttpServletRequestTextMap(request));

add ugly null check

SpanContext parent = request.getHeader("traceparent") != null ? tracer.extract(Format.Builtin.HTTP_HEADERS, new HttpServletRequestTextMap(request)) : null;

@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #698 into master will decrease coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #698      +/-   ##
============================================
- Coverage     89.70%   89.53%   -0.18%     
+ Complexity      587      586       -1     
============================================
  Files            70       70              
  Lines          2157     2159       +2     
  Branches        281      282       +1     
============================================
- Hits           1935     1933       -2     
- Misses          134      136       +2     
- Partials         88       90       +2     
Impacted Files Coverage Δ Complexity Δ
...racing/internal/propagation/TraceContextCodec.java 77.41% <100.00%> (-5.92%) 15.00 <0.00> (-1.00)

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 3f08eca...834b95a. Read the comment docs.

TextMapCodec.extract() return null with no warning if SPAN_CONTEXT_KEY not present, TraceContextCodec should align with it.

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

please add a unit test

@quaff
Copy link
Contributor Author

quaff commented Mar 24, 2020

@yurishkuro
Copy link
Member

Yes, but you are adding new behavior and a new expectation that no logs are produced on missing traceparent. That's not covered by the existing unit test.

@pavolloffay
Copy link
Member

@quaff could you please fix the CI?

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@pavolloffay pavolloffay merged commit b50aa15 into jaegertracing:master Mar 24, 2020
@pavolloffay
Copy link
Member

thanks @quaff

@quaff
Copy link
Contributor Author

quaff commented Mar 24, 2020

I admit the unit test is a bit fragile, It depends gradle, since gradle reassigned System.out, maybe there is a better way to detect log message, please improve it. @pavolloffay

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