router: per-cluster histograms w/ timeout budget#9227
Merged
mattklein123 merged 42 commits intoenvoyproxy:masterfrom Jan 11, 2020
Merged
router: per-cluster histograms w/ timeout budget#9227mattklein123 merged 42 commits intoenvoyproxy:masterfrom
mattklein123 merged 42 commits intoenvoyproxy:masterfrom
Conversation
This change adds a pair of histograms to track timeout budget usage: one for the per-request timeout and one for the global timeout. The histograms are scaled such that 100% of the timeout budget is a value of 10000 so that four digits of the percentage used can be recorded. Risk Level: Low (new stats only). Testing: Unit tests added/modified. Docs Changes: Added notes about historgrams. Release Notes: Added comment with link to docs. Fixes envoyproxy#6122 Signed-off-by: Matthew Gumport <[email protected]>
tonya11en
requested changes
Dec 4, 2019
Member
tonya11en
left a comment
tonya11en
left a comment
There was a problem hiding this comment.
Thanks for getting the ball rolling on this! I mostly have a few questions/nits this pass.
docs/root/configuration/upstream/cluster_manager/cluster_stats.rst
Outdated
Show resolved
Hide resolved
docs/root/configuration/upstream/cluster_manager/cluster_stats.rst
Outdated
Show resolved
Hide resolved
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
gave up on that approach and should've gotten them before the review. Signed-off-by: Matthew Gumport <[email protected]>
personally hurt that this is part of ci Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
I'm going to agree with ci. Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
tonya11en
requested changes
Dec 6, 2019
Member
tonya11en
left a comment
tonya11en
left a comment
There was a problem hiding this comment.
LG in general. Just one final set of comments and then we can rope in the maintainers.
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
Signed-off-by: Matthew Gumport <[email protected]>
tonya11en
previously approved these changes
Dec 10, 2019
Member
tonya11en
left a comment
tonya11en
left a comment
There was a problem hiding this comment.
LGTM modulo the nit, but that's your call. I think a maintainer should take a look now.
Member
|
Also, can you write a bit about the flag you added that toggles the histograms in the description. |
mattklein123
requested changes
Dec 11, 2019
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Nice! LGTM with some small comments. Thank you.
/wait
docs/root/configuration/upstream/cluster_manager/cluster_stats.rst
Outdated
Show resolved
Hide resolved
Signed-off-by: Matthew Gumport <[email protected]>
Member
Author
|
Going to re-merge master for the stats tests. /wait |
…-stat Signed-off-by: Matthew Gumport <[email protected]>
Member
Author
|
Still waiting for new values. /wait |
Signed-off-by: Matthew Gumport <[email protected]>
Member
Author
|
Going to merge master again. |
…-stat Signed-off-by: Matthew Gumport <[email protected]>
Member
Author
|
/wait |
Signed-off-by: Matthew Gumport <[email protected]>
Member
Author
|
Test integration foobar /wait |
gumpt
commented
Jan 10, 2020
Signed-off-by: Matthew Gumport <[email protected]>
mattklein123
requested changes
Jan 10, 2020
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM modulo merge issues. Nice work!
/wait
Signed-off-by: Matthew Gumport <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change adds a pair of histograms to track timeout budget usage: one for the
per-request timeout and one for the global timeout. The histograms are scaled
such that 100% of the timeout budget used (a timeout) is a value of 100.
The histograms by default are off. They can be turned on by setting
track_timeout_budgetson a cluster to true.Risk Level: Low (new stats only).
Testing: Unit tests added/modified.
Docs Changes: Added notes about histograms.
Release Notes: Added comment with link to docs.
Fixes #6122
Signed-off-by: Matthew Gumport [email protected]