Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add an outcome tag for web client metrics similar to MVC and WebFlux server metrics #15594

Closed
wants to merge 1 commit into from
Closed

Add an outcome tag for web client metrics similar to MVC and WebFlux server metrics #15594

wants to merge 1 commit into from

Conversation

nishantraut
Copy link
Contributor

@nishantraut nishantraut commented Dec 31, 2018 •
edited by philwebb
Loading

See #15420

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 31, 2018
@nishantraut
Copy link
Contributor Author

New test file is already formatted but still it is failing with formatting violations, can someone help me to understand what i am doing wrong here.

@snicoll
Copy link
Member

snicoll commented Dec 31, 2018 •
edited
Loading

@nishantraut I am assuming you are on Windows and have not configured your client to properly format line endings as CRLF. See, for instance, this thread.

On a related note, your copy of the repository is not clean (see all the Merge from spring-projects/master) so you may want to fix that as well. If you need some help, please ask on StackOverflow.

Please do not open a duplicate PR if you intend to fix this. You can push force on your branch and the PR will update itself automatically

@snicoll snicoll mentioned this pull request Dec 31, 2018
Closed
@nishantraut
Copy link
Contributor Author

@nishantraut I am assuming you are on Windows and have not configured your client to properly format line endings as CRLF. See, for instance, this thread.

On a related note, your copy of the repository is not clean (see all the Merge from spring-projects/master) so you may want to fix that as well. If you need some help, please ask on StackOverflow.

Please do not open a duplicate PR if you intend to fix this. You can push force on your branch and the PR will update itself automatically

Thanks @snicoll yes i am on windows applied core.autocrlf true hope it will fix.

I still need to figure out what is the best way to update my master branch as i do using compare-across forks by raising reverse PR. Thanks i will check/ask on stakcoverflow

@michaelmcfadyen
Copy link
Contributor

What would be the outcome tag value when the client call times out?

@nishantraut
Copy link
Contributor Author

It should be SERVER_ERROR

@nishantraut
Copy link
Contributor Author

Sorry i will try to fix formatting issue on weekend. Apologize for inconvenience.

@michaelmcfadyen
Copy link
Contributor

It should be SERVER_ERROR

Is that suitable? Server error would indicate a 5xx status response from the client. In the case of a timeout there is no response from the client.

@nishantraut
Copy link
Contributor Author

Hi @michaelmcfadyen, Depends what status code is returned if it is 4xx (guess client timeout is 408) then CLIENT_ERROR or 5xx then SERVER_ERROR. This changes are aligned with WebMvc and WebFlux outcomes.

@philwebb philwebb changed the title Add an outcome tag to RestTemplate metrics similar to that provided for WebMVC and WebFlux #15420 Add an outcome tag to RestTemplate metrics similar to that provided for WebMVC and WebFlux Jan 2, 2019
@michaelmcfadyen
Copy link
Contributor

In the case of the http call timing out (read/connect) there will be no status in the response. In my opinion, setting the outcome tag as SERVER_ERROR for a timeout from the http client is misleading.

@nishantraut
Copy link
Contributor Author

nishantraut commented Jan 3, 2019 •
edited
Loading

I am quite new to this code base, can someone from springboot team revert on above question

@nishantraut
Copy link
Contributor Author

@nishantraut I am assuming you are on Windows and have not configured your client to properly format line endings as CRLF. See, for instance, this thread.

On a related note, your copy of the repository is not clean (see all the Merge from spring-projects/master) so you may want to fix that as well. If you need some help, please ask on StackOverflow.

Please do not open a duplicate PR if you intend to fix this. You can push force on your branch and the PR will update itself automatically

Hi @snicoll I have cleaned up unnecessary commits and merged into one, can you please review PR and merge if it looks ok. Thank you for the support.

@bclozel bclozel self-assigned this Jan 7, 2019
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 8, 2019
@bclozel bclozel added this to the 2.2.0.M1 milestone Jan 8, 2019
bclozel pushed a commit that referenced this pull request Jan 8, 2019
@nishantraut @bclozel
Add an outcome tag to web client metrics
21df40b 
Similar to what's ben done in gh-15420 for Spring MVC and Spring
WebFlux, this commit adds an outcome tag for the client side on both
`RestTemplate` and `WebClient`.

See gh-15594
@bclozel bclozel closed this in d5ae59d Jan 8, 2019
@bclozel
Copy link
Member

bclozel commented Jan 8, 2019

Thanks for your contribution @nishantraut - I've polished things a bit in d5ae59d to address the comments here - if the response status cannot be resolved, we'll settle with UNKNOWN for the outcome tag information. I've also refined the error cases around status resolution.

@bclozel bclozel changed the title Add an outcome tag to RestTemplate metrics similar to that provided for WebMVC and WebFlux Add an outcome tag for web client metrics similar to MVC and WebFlux server metrics Jan 8, 2019
@snicoll snicoll mentioned this pull request Jan 9, 2019
Closed
michaelmcfadyen added a commit to michaelmcfadyen/spring-boot that referenced this pull request Jan 16, 2019
@michaelmcfadyen
Update 'HTTP Client Metrics' section of the documentation to contain …
c55a68b 
…information on the 'outcome' tag introduced as part of spring-projects#15594. Also, update the layout of the tags section to match other similar sections
@michaelmcfadyen michaelmcfadyen mentioned this pull request Jan 16, 2019
Closed
@bclozel bclozel mentioned this pull request Oct 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bclozel bclozel

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.2.0.M1
Development

Successfully merging this pull request may close these issues.
5 participants
@nishantraut @snicoll @michaelmcfadyen @bclozel @spring-projects-issues