Skip to content

thrift_proxy: Add zone response metrics to thrift upstream requests#18512

Merged
davinci26 merged 9 commits intoenvoyproxy:mainfrom
fishcakez:thrift-zone
Oct 12, 2021
Merged

thrift_proxy: Add zone response metrics to thrift upstream requests#18512
davinci26 merged 9 commits intoenvoyproxy:mainfrom
fishcakez:thrift-zone

Conversation

@fishcakez
Copy link
Copy Markdown
Contributor

Commit Message: Add zone response metrics to thrift upstream requests
Additional Description: Add response type/reply type counters and request time per zone pair upstream metrics in the same format as http zone metrics.
Risk Level: low
Testing: unit tests and local deployment
Docs Changes: N/A
Release Notes: Yes, added metrics.
Platform Specific Features: N/A

@fishcakez fishcakez requested a review from zuercher as a code owner October 8, 2021 00:44
Signed-off-by: James Fish <jfish@pinterest.com>
Signed-off-by: James Fish <jfish@pinterest.com>
Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

Looks good once we get the formatting passing, thanks!

Signed-off-by: James Fish <jfish@pinterest.com>
Signed-off-by: James Fish <jfish@pinterest.com>
Signed-off-by: James Fish <jfish@pinterest.com>
Signed-off-by: James Fish <jfish@pinterest.com>
Signed-off-by: James Fish <jfish@pinterest.com>
@rgs1
Copy link
Copy Markdown
Member

rgs1 commented Oct 10, 2021

@fishcakez almost there, small drop in coverage:

Per-extension coverage failed:
Code coverage for source/extensions/filters/network/thrift_proxy/router is lower than limit of 96.4 (96.2)
##[error]Bash exited with code '1'.

https://dev.azure.com/cncf/envoy/_build/results?buildId=91332&view=logs&jobId=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881

https://storage.googleapis.com/envoy-pr/ca0196f/coverage/source/extensions/filters/network/thrift_proxy/router/index.html

Signed-off-by: James Fish <jfish@pinterest.com>
@fishcakez
Copy link
Copy Markdown
Contributor Author

small drop in coverage

From the output you shared the drop is due to adding some empty lines, which get reported as 0 counts, so I just deleted 4 of the ones I introduced.

@davinci26
Copy link
Copy Markdown
Member

Assigning myself for final review after the thrift folks approve.

Signed-off-by: James Fish <jfish@pinterest.com>
Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Over to @davinci26 for final pass & merge.

Copy link
Copy Markdown
Member

@davinci26 davinci26 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@davinci26 davinci26 merged commit 943e983 into envoyproxy:main Oct 12, 2021
@fishcakez fishcakez deleted the thrift-zone branch October 12, 2021 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants