Skip to content

Return 0 status code on exception or invalid code#1387

Merged
marcingrzejszczak merged 2 commits into2.1.xfrom
issues_#1382_invalid_code
Jul 3, 2019
Merged

Return 0 status code on exception or invalid code#1387
marcingrzejszczak merged 2 commits into2.1.xfrom
issues_#1382_invalid_code

Conversation

@marcingrzejszczak
Copy link
Contributor

fixes gh-1382

@Override
public Integer statusCode(ClientResponse response) {
return response.statusCode().value();
return response.rawStatusCode();
Copy link
Contributor

Choose a reason for hiding this comment

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

something like this otherwise you still end up with the exception :P

  @Override public Integer statusCode(HttpResponse response) {
    int result = statusCodeAsInt(response);
    return result != 0 ? result : null;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok but statusCode is called only via the statusCodeAsInt in Brave

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #1387 into 2.1.x will increase coverage by 0.22%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##              2.1.x   #1387      +/-   ##
===========================================
+ Coverage     66.78%     67%   +0.22%     
  Complexity      850     850              
===========================================
  Files           145     145              
  Lines          4028    4031       +3     
  Branches        422     422              
===========================================
+ Hits           2690    2701      +11     
+ Misses         1099    1090       -9     
- Partials        239     240       +1
Impacted Files Coverage Δ Complexity Δ
...nt/web/client/TraceWebClientBeanPostProcessor.java 77.69% <100%> (+2.69%) 9 <0> (ø) ⬇️
...th/instrument/async/ExecutorBeanPostProcessor.java 69.74% <50%> (ø) 38 <0> (ø) ⬇️
...ing/TracingConnectionFactoryBeanPostProcessor.java 72.13% <0%> (+8.19%) 6% <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 fd35911...052d14a. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #1387 into 2.1.x will decrease coverage by 0.16%.
The diff coverage is 57.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.1.x    #1387      +/-   ##
============================================
- Coverage     66.78%   66.61%   -0.17%     
  Complexity      850      850              
============================================
  Files           145      145              
  Lines          4028     4032       +4     
  Branches        422      423       +1     
============================================
- Hits           2690     2686       -4     
- Misses         1099     1108       +9     
+ Partials        239      238       -1
Impacted Files Coverage Δ Complexity Δ
...th/instrument/async/ExecutorBeanPostProcessor.java 69.74% <50%> (ø) 38 <0> (ø) ⬇️
...nt/web/client/TraceWebClientBeanPostProcessor.java 74.28% <60%> (-0.72%) 9 <0> (ø)
...jms/config/TracingJmsListenerEndpointRegistry.java 60.91% <0%> (-6.9%) 17% <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 fd35911...c7b37a1. Read the comment docs.

@marcingrzejszczak marcingrzejszczak merged commit 94c7c22 into 2.1.x Jul 3, 2019
codefromthecrypt pushed a commit to openzipkin/brave that referenced this pull request Jul 3, 2019
Eventhough new instrumentation don't use it, old could.

See spring-cloud/spring-cloud-sleuth#1387
@marcingrzejszczak marcingrzejszczak deleted the issues_#1382_invalid_code branch June 16, 2020 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants