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

Improve partial error logging for Elasticsearch meter registry #1418

Merged
merged 1 commit into from
May 17, 2019
Merged

Improve partial error logging for Elasticsearch meter registry #1418

merged 1 commit into from
May 17, 2019

Conversation

izeye
Copy link
Contributor

@izeye izeye commented May 14, 2019

When the HTTP status is considered as success but its body has errors property, there's no logging for request body unlike unsuccessful HTTP statuses. This PR changes to log HTTP request errors in Elasticsearch meter registry consistently.

@shakuzen
Copy link
Member

Are you aware of Elasticsearch documentation on this? I would like to understand if they have documented differences of when Elasticsearch will return an unsuccessful status code versus a successful status code with an unsuccessful body.

@shakuzen shakuzen added the registry: elastic An ElasticSearch Registry related issue label May 15, 2019
@izeye
Copy link
Contributor Author

izeye commented May 15, 2019

@shakuzen I didn't find any doc on it but when I was looking into #1071 I noticed the errors property has been set when there's partial failure of a bulk request.

With regards to the error property case I'd like to change the log message to show numbers for both successes and failures as the current message seems to indicate a total failure. If you agree with this, I can update this PR or prepare another one for it.

@shakuzen
Copy link
Member

I didn't find any doc on it but when I was looking into #1071 I noticed the errors property has been set when there's partial failure of a bulk request.

With regards to the error property case I'd like to change the log message to show numbers for both successes and failures as the current message seems to indicate a total failure. If you agree with this, I can update this PR or prepare another one for it.

Yes that seems to make sense. Otherwise partial failures and complete failures might look the same.

@izeye
Copy link
Contributor Author

izeye commented May 15, 2019

@shakuzen Thanks for the feedback!

If you agree with this, I can update this PR or prepare another one for it.

Is it okay to update this PR accordingly?

@shakuzen
Copy link
Member

Is it okay to update this PR accordingly?

Yes, please.

@izeye izeye force-pushed the elastic-http-request-error-logging branch from 75a8f07 to 4461892 Compare May 15, 2019 18:26
@izeye izeye force-pushed the elastic-http-request-error-logging branch from 4461892 to 41d62d2 Compare May 15, 2019 18:29
@izeye
Copy link
Contributor Author

izeye commented May 15, 2019

@shakuzen I updated as discussed.

@izeye izeye changed the title Log HTTP request errors in Elasticsearch meter registry consistently Improve partial error logging for Elasticsearch meter registry May 15, 2019
@shakuzen shakuzen added this to the 1.1.5 milestone May 17, 2019
@shakuzen shakuzen added the enhancement A general enhancement label May 17, 2019
@shakuzen shakuzen merged commit de39a24 into micrometer-metrics:1.1.x May 17, 2019
@izeye izeye deleted the elastic-http-request-error-logging branch May 17, 2019 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shakuzen shakuzen shakuzen approved these changes

Assignees
No one assigned
Labels
enhancement A general enhancement registry: elastic An ElasticSearch Registry related issue
Projects
None yet
Milestone
1.1.5
Development

Successfully merging this pull request may close these issues.
2 participants
@izeye @shakuzen