thrift_proxy: add upstream cluster metrics for MessageType#15668
Merged
mattklein123 merged 14 commits intoenvoyproxy:mainfrom Apr 6, 2021
Merged
thrift_proxy: add upstream cluster metrics for MessageType#15668mattklein123 merged 14 commits intoenvoyproxy:mainfrom
mattklein123 merged 14 commits intoenvoyproxy:mainfrom
Conversation
Contributor
Author
|
cc @rgs1 @fishcakez |
rgs1
suggested changes
Mar 25, 2021
Member
rgs1
left a comment
rgs1
left a comment
There was a problem hiding this comment.
LGTM, thanks Will! Left an inline comment plus two additional asks:
- can you document the new stats in docs/root/configuration/other_protocols/thrift_filters/router_filter.rst
- can you also add a changelog entry to docs/root/version_history/current.rst
source/extensions/filters/network/thrift_proxy/router/router_impl.h
Outdated
Show resolved
Hide resolved
Signed-off-by: William Fu <wfu@pinterest.com> Do not pass cluster scope around Signed-off-by: William Fu <wfu@pinterest.com> Update rst files Signed-off-by: William Fu <wfu@pinterest.com>
williamsfu99
force-pushed
the
thrift_upstream
branch
from
89b6f91 to
7c451ec
Compare
added 3 commits
March 26, 2021 10:31
Signed-off-by: William Fu <wfu@pinterest.com>
Signed-off-by: William Fu <wfu@pinterest.com>
Signed-off-by: William Fu <wfu@pinterest.com>
rgs1
reviewed
Mar 26, 2021
docs/root/configuration/other_protocols/thrift_filters/router_filter.rst
Show resolved
Hide resolved
added 3 commits
March 26, 2021 10:40
Signed-off-by: William Fu <wfu@pinterest.com>
Signed-off-by: William Fu <wfu@pinterest.com>
rgs1
reviewed
Mar 28, 2021
Member
rgs1
left a comment
rgs1
left a comment
There was a problem hiding this comment.
Looks great -- thanks for the update, I have one more question.
| COUNTER(request_invalid_type) \ | ||
| COUNTER(response_reply) \ | ||
| COUNTER(response_exception) \ | ||
| COUNTER(response_invalid_type) |
Member
There was a problem hiding this comment.
while we are here, do we also want to bytes per request/response?
Contributor
Author
There was a problem hiding this comment.
I can do that in a follow-up PR, I just want to cover messageTypes for now (as we don't have any per-upstream metrics to begin with).
Signed-off-by: William Fu <wfu@pinterest.com>
added 3 commits
March 30, 2021 11:23
…thrift_upstream Signed-off-by: William Fu <wfu@pinterest.com>
Signed-off-by: William Fu <wfu@pinterest.com>
Signed-off-by: William Fu <wfu@pinterest.com>
Signed-off-by: William Fu <wfu@pinterest.com>
zuercher
reviewed
Apr 5, 2021
source/extensions/filters/network/thrift_proxy/router/router_impl.h
Outdated
Show resolved
Hide resolved
Signed-off-by: William Fu <wfu@pinterest.com>
Signed-off-by: William Fu <wfu@pinterest.com>
Member
|
@rgs1 can you have another look since a couple things changed? |
zuercher
pushed a commit
that referenced
this pull request
Apr 14, 2021
) In #15668 we began supporting cluster level metrics for the thrift_proxy connection manager. These metrics were limited to messageType counters only; now that these are working OK we are comfortable moving to cluster level histograms for upstream_rq_time. Risk Level: Low Testing: New unit tests Docs Changes: future pr Release Notes: updated Signed-off-by: William Fu <wfu@pinterest.com>
douglas-reid
pushed a commit
to douglas-reid/envoy
that referenced
this pull request
Apr 19, 2021
…oyproxy#15884) In envoyproxy#15668 we began supporting cluster level metrics for the thrift_proxy connection manager. These metrics were limited to messageType counters only; now that these are working OK we are comfortable moving to cluster level histograms for upstream_rq_time. Risk Level: Low Testing: New unit tests Docs Changes: future pr Release Notes: updated Signed-off-by: William Fu <wfu@pinterest.com> Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
Monkeyanator
pushed a commit
to Monkeyanator/envoy
that referenced
this pull request
Apr 20, 2021
…oyproxy#15884) In envoyproxy#15668 we began supporting cluster level metrics for the thrift_proxy connection manager. These metrics were limited to messageType counters only; now that these are working OK we are comfortable moving to cluster level histograms for upstream_rq_time. Risk Level: Low Testing: New unit tests Docs Changes: future pr Release Notes: updated Signed-off-by: William Fu <wfu@pinterest.com>
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.
Signed-off-by: William Fu wfu@pinterest.com
We have messagetype thrift metrics on the listener side, aka thrift_proxy's connection_manager implementation, but it is also useful to distinguish these counters across each upstream when we have multiple thrift upstreams handled by the router.
Commit Message: thrift_proxy: add upstream cluster metrics for MessageType
Additional Description:
Risk Level: medium (affects thrift customers)
Testing: new unit tests
Docs Changes:
Release Notes:
Platform Specific Features: