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.

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

Automated cherry pick of detailed "took too long" warnings to release-3.2 #9821

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Jun 7, 2018

Cherry pick request "took too long" warning improvements to release-3.2.

To improve debuggability of etcd. This is important since etcd 3.1+ no longer appended quorum writes to WAL log, and this will the only record expensive requests.

#9288: etcdserver: improve request took too long warning
#9661: etcdserver: not print password in the warning message of expensive request
#9822: etcdserver: Replace value contents with value_size in request took too long warning
#9826: etcdserver: Add response byte size and range response count to took too long warning
#9835: etcdserver: Fix txn request 'took too long' warnings to use loggable request stringer

@jpbetz
Copy link
Contributor Author

jpbetz commented Jun 7, 2018

After discussing with @gyuho, we'd like to only log the payload size, and not the payload contents, so we'll first fix that on master and then cherrypick it back.

This PR will be on hold until we sort that out.

@gyuho
Copy link
Contributor

gyuho commented Jun 7, 2018

@jpbetz Sounds good. Right now, we manually redact password field only for etcd #9661, but user may have sensitive data in its payload. And I notice these warnings are really hard to read with large values (dumping 1 MB plaintext to stderr). /cc @xiang90

@jpbetz
Copy link
Contributor Author

jpbetz commented Jun 7, 2018

@gyuho Ah, I need that as well. I'll augment raft_internal_stringer.go to also replace value fields (of type []byte) with value_size (of type int64).

@jpbetz jpbetz force-pushed the automated-cherry-pick-of-#9288-origin-release-3.2 branch 2 times, most recently from 5c6f444 to 18a8737 Compare June 12, 2018 00:01
@jpbetz jpbetz changed the title Automated cherry pick of #9288 Automated cherry pick of detailed "took too long" warnings to release-3.2 Jun 12, 2018
@gyuho
Copy link
Contributor

gyuho commented Jun 12, 2018

@jpbetz Do we also want #9835 to be backported (I did for 3.3 branch)? Otherwise, lgtm?

@jpbetz jpbetz force-pushed the automated-cherry-pick-of-#9288-origin-release-3.2 branch from 18a8737 to 0ce2ef1 Compare June 12, 2018 19:35
@jpbetz
Copy link
Contributor Author

jpbetz commented Jun 12, 2018

@gyuho Thanks, yes, I had manually fixed this backport with the same fix as in #9835, but it's cleaner to actually cherrypick that exact #9835 fix back so all the attribution and future backports are as clean as possible, so I've done that.

@jpbetz
Copy link
Contributor Author

jpbetz commented Jun 12, 2018

I'll send out a 3.3 backport shortly.

@gyuho
Copy link
Contributor

gyuho commented Jun 12, 2018

@jpbetz Oh, those are already backported to 3.3 yesterday :)

LGTM. thanks!

@gyuho gyuho merged commit ba23379 into etcd-io:release-3.2 Jun 12, 2018
gyuho referenced this pull request Jun 12, 2018
…rigin-release-3.1-1528833932

etcdserver: Automated cherry pick of detailed "took too long" warnings to release-3.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants