-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Support Gauge in access_loggers.stats #42226
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
Signed-off-by: Xuyang Tao <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
mattklein123
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.
/wait
| // A fixed value to add to this counter. | ||
| // One of ``value_format`` or ``value_fixed`` must be configured. |
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 comment needs to be updated for how it relates to the gauge operations.
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.
updated.
Signed-off-by: Xuyang Tao <[email protected]>
ggreenway
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.
/wait
| google.protobuf.UInt64Value value_fixed = 3 [(validate.rules).uint64 = {gt: 0}]; | ||
| } | ||
|
|
||
| // Configuration for a gauge stat. |
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 think it's quite tricky to use a gauge correctly (to avoid "leaking" of counts). Please add documentation on correct and incorrect ways this could be configured, with some examples.
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.
updated.
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
|
|
||
| // The access log type to emit this gauge. If set, the gauge will only be | ||
| // emitted for the specified access log type. | ||
| data.accesslog.v3.AccessLogType access_log_type = 5; |
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 seems like it should be common to all types (counters and histograms). Should this be part of message Stat? Or should it be a top-level configuration on the logger, and user defines multiple loggers, one for each desired value?
Commit Message: add gauge support in access_loggers.stats
Risk Level:low
Testing:both unit test and integration test are added
Docs Changes: updated in the protobuf comments
Release Notes: updated
Platform Specific Features:no