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

Closed
wants to merge 4 commits into 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
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 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.

@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
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.

LGTM

message StacktraceFilter {
optional StacktraceFilterFunctionName function_name = 1;

// StacktraceFilterFunctionNameRegExp functionNameRegExp = 2;
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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:

Suggested change
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;

@alsoba13 alsoba13 closed this Jun 20, 2025
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