Skip to content
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

feat: fetch recording rules from tenant-settings #3874

Open
wants to merge 2 commits into
base: alsoba13/metrics-from-profiles-record-and-export
Choose a base branch
from

Conversation

alsoba13
Copy link
Contributor

This is a draft. Plenty of assumptions have been done. WIP

@alsoba13 alsoba13 requested a review from a team as a code owner January 30, 2025 20:47
for _, matcher := range rec.rule.matchers {
exportedLabels = append(exportedLabels, labels.Label{
Name: matcher.Name,
Value: matcher.Value,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was in fact a bug. I was appending name value pair, when the label value may not be the label filter value (for exmaple vehicle!="car")-

Anyway, considering removing this part as the code comment argues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I finally removed.

@@ -135,7 +129,7 @@ func generateExportedLabels(labelsMap map[string]string, rec *Recording, pyrosco
}

func (r *Recording) matches(labelsMap map[string]string) bool {
if r.rule.profileType != labelsMap["__profile_type__"] {
if r.rule.profileType != labelsMap["__type__"] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: solve ambiguities: __type__ is not enough! block/contentions and block/delays can't be distinguish from mutex/contentions and mutex/delays

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 your work on this. I left a couple of comments in line.

In general I think this is a bit too hacky for my liking and I would prefer for use to defined types properly in protobuf and have typed API calls to return the metric rules.

The settings API currently has mostly settings about how the UI should be presenting things (collapsing, maxNodes,...), I think this is something more complex and I think this should be a separate MetricsFromProfileRulesService as part of the settingsv1 API group.

I had done very similar things for CollectorRules: #3865

A well speced out API will make the UI integration easier and will also allow it to implement proper RBAC, API permissions going forward.

"github.com/grafana/pyroscope/api/gen/proto/go/settings/v1/settingsv1connect"
connectapi "github.com/grafana/pyroscope/pkg/api/connect"
"github.com/grafana/pyroscope/pkg/tenant"
"github.com/grafana/pyroscope/pkg/util"
)

type RecordingRule struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better if this is a protobuf defined type.

}
}

func parseMatchers(matchersString string, serviceName string) []*labels.Matcher {
Copy link
Contributor

Choose a reason for hiding this comment

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

github.com/prometheus/prometheus/promql/parser.ParseMetricSelector should be used

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