stats: Fix gRPC xDS pending_requests: now drains back to 0#5796
stats: Fix gRPC xDS pending_requests: now drains back to 0#5796htuch merged 10 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
|
Two notes: 1) I intend to move checkRateLimitAllowsDrain() to private; not doing it yet to keep the more important changes easy to see in the diff, and 2) I don't understand why the test isn't passing. I think I either don't exactly understand what the test harness is doing, or stats_store_.gauge().value() does not refer to the same stats store I'm causing a change to. |
|
@fredlas can you look into the build failures? Seems legit. Feel free to make the helper private as needed. |
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
|
Turns out I was applying the test framework correctly, and just not understanding the logic being tested! I now realize that the request queue only gets used in cases of rate limiting. Left a comment to clarify. |
Signed-off-by: Fred Douglas <fredlas@google.com>
|
After working with the guts of stats in another PR, I realized why this one was originally using set() like it was: it was avoiding activating this stat in the (common) case that rate limiting was not active. I switched from inc/dec back to set. But, the set() is now in a different place, where it will be correct. |
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
|
Please merge master to pick up #5827. |
Signed-off-by: Fred Douglas <fredlas@google.com>
|
Merged, thanks for the heads up. |
|
Ping; should be ready to go now. |
Signed-off-by: Fred Douglas <fredlas@google.com>
|
@fredlas I don't see any test change in the diff? Did a change get dropped accidentally? |
|
@mattklein123 I see comment changes in 7998d8d, which covers my last set of Qs. |
|
@htuch Shouldn't this change have a test that failed before the fix? I don't see any test changes? |
|
Whoops, yeah. I started with tests that were testing what I (wrongly) thought the logic was supposed to be, and removed them once I realized I was testing the wrong thing, but didn't add anything else. #5903 tests the fixed behavior. Sorry! |
…y#5796) xDS gRPC's stat for number of xDS request queued to be sent was previously only getting set when an attempt to send hit rate limiting. In addition to not updating when new items arrive, this stat would never return to 0 once set to non-0. Risk Level: low Testing: added coverage of fixed thing to existing unit test Signed-off-by: Fred Douglas <fredlas@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
…xy#5903) Signed-off-by: Fred Douglas <fredlas@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
3 participants
Description: xDS gRPC's stat for number of xDS request queued to be sent was previously only getting set when an attempt to send hit rate limiting. In addition to not updating when new items arrive, this stat would never return to 0 once set to non-0.
Risk Level: low
Testing: added coverage of fixed thing to existing unit test