Skip to content
New issue

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

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

Already on GitHub? # to your account

ES storage: logging number of total and failed requests #902

Conversation

tmszdmsk
Copy link
Contributor

Plz, review it as I am not golang native ;)

Which problem is this PR solving?

Short description of the changes

  • for more debug info, number of failed and total requests in bulk in case of error

Signed-off-by: Tomasz Adamski <tomasz.adamski@gmail.com>
@tmszdmsk tmszdmsk force-pushed the ES-storage-logging-nubmer-of-failed-requests branch from 756b01e to 225ff97 Compare June 30, 2018 20:41
@tmszdmsk
Copy link
Contributor Author

tmszdmsk commented Jun 30, 2018

hm, I've noticed that we are already logging response. Should we remove it also as it may contain the same number of entries as requests?

@codecov
Copy link

codecov bot commented Jun 30, 2018

Codecov Report

Merging #902 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #902   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         126      126           
  Lines        6072     6072           
=======================================
  Hits         6070     6070           
  Misses          2        2

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 2793aa8...c8dfec8. Read the comment docs.

failed := 0
if response.Failed() != nil {
failed = len(response.Failed())
}
Copy link
Member

Choose a reason for hiding this comment

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

failed := len(response.Failed()) is sufficient, since len(nil) == 0

logger.Error("Elasticsearch could not process bulk request", zap.Error(err),
zap.Any("response", response))
zap.Any("response", response), zap.Int("failed_number", failed),
Copy link
Member

Choose a reason for hiding this comment

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

  • let's put one arg per line, including error
  • rename to request_count and failed_count
  • I would order the fields: request_count, failed_count, error, response

Signed-off-by: Tomasz Adamski <tomasz.adamski@gmail.com>
@tmszdmsk tmszdmsk force-pushed the ES-storage-logging-nubmer-of-failed-requests branch from 32c8068 to c8dfec8 Compare June 30, 2018 21:12
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.

lgtm, thx

@yurishkuro yurishkuro merged commit c8d514c into jaegertracing:master Jun 30, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants