-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
ext_authz: Allow to set additional prefix for HTTP filter stats #13215
Conversation
This allows to set additional prefix for ext_authz HTTP filter. Signed-off-by: Dhi Aurrahman <[email protected]>
Signed-off-by: Dhi Aurrahman <[email protected]>
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.
Looks good, some minor comment.
/wait
// "@type": type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthz | ||
// stat_prefix: blocker # This emits ext_authz.blocker.ok, ext_authz.blocker.denied, etc. | ||
// | ||
string stat_prefix = 13; |
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.
This is consistent with what we do in HCM, so seems totally fair. I could imagine having a more generic filter wrapper that included a stat prefix, but I think that's total overkill at this point.
Signed-off-by: Dhi Aurrahman <[email protected]>
Signed-off-by: Dhi Aurrahman <[email protected]>
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.
LGTM, thanks!
@dio needs master merge :) |
Thanks for the review, @htuch! Merged master to this branch. :) |
Commit Message:
This patch allows setting an additional prefix for HTTP filter stats. This lets the emitted statistics from configured ext_authz HTTP filters in an HTTP filter chain can be distinguished from each other.
Risk Level: Low
Testing: Added a test on additional prefix.
Docs Changes: Added
Release Notes: Added
Fixes #12666