Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alsoba13
Copy link
Contributor

@alsoba13 alsoba13 commented Apr 16, 2025

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:

# total cpu per service
- metric_name: 'pyroscope_metric_cpu_total_nanoseconds'
  matchers: ['{__profile_type__="..."}']
  group_by: ['service_name']
# garbage collection total cpu usage per service
- metric_name: 'pyroscope_metric_ingester_cpu_gc_total_nanoseconds'
  matchers: ['{__profile_type__="..."}']
  group_by: ['service_name']
  stacktrace_filter:
    type: 'function_name'
    function_name: 'runtime.gcBgMarkWorker'
    metric_type: total

With this, we should be able to extend it easily to: self function names, and self/total reg_exp function names

@alsoba13 alsoba13 requested a review from a team as a code owner April 16, 2025 08:54
@@ -74,11 +74,34 @@ message RecordingRule {
repeated string matchers = 4;
repeated string group_by = 5;
repeated types.v1.LabelPair external_labels = 6;
StacktraceFilter stacktrace_filter = 7;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional. In absence of it total dimension (current implementation) is used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally that should take the optional argument

Suggested change
StacktraceFilter stacktrace_filter = 7;
optional StacktraceFilter stacktrace_filter = 7;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess changes you make here also need to be done to message RecordingRuleStore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I need to make generate too. Will do it now

Copy link
Contributor

@simonswine simonswine left a 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

Comment on lines 86 to 87
StacktraceFilterType type = 1;
StacktraceFilterFunctionName function_name = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than type I think it is nicer to use optional field per type and validate that only one is enabled at a time.

Alternatively there is also oneOf in protobuf: https://protobuf.dev/programming-guides/proto3/#using-oneof

Suggested change
StacktraceFilterType type = 1;
StacktraceFilterFunctionName function_name = 2;
optional StacktraceFilterFunctionName function_name = 1;
// And in the future
// optional StacktraceFilterFunctionNameRegExp functionNameRegExp = 2;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I like this approach as well. I'll do it this way!

(I copied mine from Query)

Comment on lines 77 to 82
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;
Copy link
Contributor

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.

@@ -74,11 +74,34 @@ message RecordingRule {
repeated string matchers = 4;
repeated string group_by = 5;
repeated types.v1.LabelPair external_labels = 6;
StacktraceFilter stacktrace_filter = 7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally that should take the optional argument

Suggested change
StacktraceFilter stacktrace_filter = 7;
optional StacktraceFilter stacktrace_filter = 7;

@@ -74,11 +74,34 @@ message RecordingRule {
repeated string matchers = 4;
repeated string group_by = 5;
repeated types.v1.LabelPair external_labels = 6;
StacktraceFilter stacktrace_filter = 7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess changes you make here also need to be done to message RecordingRuleStore?

@alsoba13 alsoba13 force-pushed the alsoba13/function-totals-rules-proto branch from d3cebd9 to 9f50340 Compare May 2, 2025 08:23
@alsoba13 alsoba13 force-pushed the alsoba13/function-totals-rules-proto branch from 9f50340 to 639e51e Compare May 2, 2025 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants