-
Notifications
You must be signed in to change notification settings - Fork 5.2k
access_logger: add new logger to record metrics derived from substitution formatter values #41770
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
Conversation
Signed-off-by: Greg Greenway <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Greg Greenway <[email protected]>
api/envoy/extensions/filters/http/timing_stats/v3/timing_stats.proto
Outdated
Show resolved
Hide resolved
api/envoy/extensions/filters/http/timing_stats/v3/timing_stats.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Greg Greenway <[email protected]>
api/envoy/extensions/filters/http/timing_stats/v3/timing_stats.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
|
Starting to add some implementation. Completely untested so far. I think I'm doing the StatName storage correctly, but please take a look and let me know. |
wbpcode
left a comment
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.
Thanks for this great contribution. Some initial comments to the API are added.
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/41770/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
|
@wbpcode @kyessenov tests and docs are written. This is now fully ready for review. |
| getFormatValue(*histogram.value_formatter_, context, stream_info, | ||
| histogram.unit_ == Stats::Histogram::Unit::Percent); | ||
| if (!computed_value_opt.has_value()) { | ||
| continue; |
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.
Might be useful to add a failure stat? Given we log, not necessary though.
|
LGTM and some feedback. Nothing that can't be fixed later though. |
Signed-off-by: Greg Greenway <[email protected]>
kyessenov
left a comment
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.
Thank you!
|
/retest |
|
@wbpcode or @adisuissa this needs an API signoff |
wbpcode
left a comment
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 api
|
@ggreenway Thanks so much for this PR. This will be a huge change for our users/stats system. 👍 |
wbpcode
left a comment
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.
Code also LGTM. Thanks.
|
I think it would be better to have a detailed document/example to show how it works. But it needn't to be done in this PR anyway. |
adisuissa
left a comment
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.
Thanks overal LGTM.
On thing I suggest clarifying is what happens if there's a conflict between header0set stat name and one that Envoy already has.
/lgtm api
I suggest adding an example showing how to use this extension (either in the api or accompanying doc).
|
/retest |
Commit Message: Added a new access logger which emits configurable metrics.
Risk Level: Low
Testing: New tests added
Docs Changes: Docs in new proto
Release Notes: added
Platform Specific Features: None
Fixes #34168