-
Notifications
You must be signed in to change notification settings - Fork 675
feat(v2): function totals recording rules #4107
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
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 working on this. A few suggestions, let me know once you want me to take another look
StacktraceFilter stacktrace_filter = 7; | ||
|
||
// The observed generation of this recording rule. This value should be | ||
// provided when making updates to this record, to avoid conflicting | ||
// concurrent updates. | ||
int64 generation = 7; | ||
int64 generation = 8; |
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.
Careful in protobuf those field IDs should never change or be reused after deprecation, as that would break existing API users.
d3cebd9
to
9f50340
Compare
9f50340
to
639e51e
Compare
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
message StacktraceFilter { | ||
optional StacktraceFilterFunctionName function_name = 1; | ||
|
||
// StacktraceFilterFunctionNameRegExp functionNameRegExp = 2; |
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 would remove those comments, they might just mislead users
enum MetricType { | ||
TOTAL = 0; | ||
|
||
// SELF = 1; |
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 would remove those comments, they might just mislead users
@@ -79,6 +81,25 @@ message RecordingRule { | |||
// provided when making updates to this record, to avoid conflicting | |||
// concurrent updates. | |||
int64 generation = 7; | |||
|
|||
optional StacktraceFilter stacktrace_filter = 8; |
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.
Good opportunity for a comment:
optional StacktraceFilter stacktrace_filter = 8; | |
// The stacktrace filter allows filtering on particular function names in the stacktrace. | |
// This allows recording rules to focus on specific functions and calculate their "total" | |
// resource usage. | |
optional StacktraceFilter stacktrace_filter = 8; |
Adding a stacktrace filter field in the recording rules.
This way, we can define filter for function names and extend it in the future.
Examples of use:
With this, we should be able to extend it easily to: self function names, and self/total reg_exp function names