-
Notifications
You must be signed in to change notification settings - Fork 608
feat: add default stats_tags to improve the prometheus metrics #7701
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7701 +/- ##
==========================================
+ Coverage 72.28% 72.30% +0.02%
==========================================
Files 234 234
Lines 34480 34480
==========================================
+ Hits 24924 24931 +7
+ Misses 7767 7760 -7
Partials 1789 1789 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| stats_config: | ||
| use_all_default_tags: true | ||
| stats_tags: | ||
| - regex: \.zone(\.(([^\.]+)\.)) | ||
| tag_name: from_zone | ||
| - regex: \.zone\.[^\.]+\.(([^\.]+)\.) | ||
| tag_name: to_zone | ||
| - regex: "^cluster(\\..+\\.(.+))\\.total_match_count$" | ||
| tag_name: socket_match_name | ||
| - regex: "circuit_breakers\\.((.+?)\\.).+" | ||
| tag_name: priority |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would it look like for a user to opt-out from this? One consideration is that zone metrics are likely only relevant when using Zone Aware Routing and would only work if TopologyInjector is enabled (which is the default so not a huge deal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's something we need to figure out.
This PR made some breaking changes, which IMO it's good one, should be and a feature gate for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By breaking change you mean the metric name right? That doesn't seem too disruptive so as long as it's mentioned in the release notes I'm still okay with it.
Having this as the default seems reasonable so I don't think a feature gate is necessary. Users could modify the EnvoyProxy config to revert this if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm 100% agree with you.
cc @envoyproxy/gateway-maintainers PTAL
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
|
@zirain can you add a diff of before/after to the PR description, so its easier to understand the breaking stat changes |
updated |
xref: #7606
add default
stats_tagconfig to improve the prometheus metric output.to_zoneandfrom_zonesocket_match_namepriorityBefore:
before.txt
After:
after.txt