add new parameter "group" to filter alert rules

[gunzl1ng3r]

What it does:

• adds new parameter -g/--group that allows to filter AlertRules based on their group

Why is it required:

• Our Prometheus installation comes with a lot of default Alerts, that we cannot edit and which are therefore useless; excluding them using the new feature --exclude-alert is not viable...

What else:

• To make the data used in cmd/alert_test.go more readable, I moved the single-line jsons to variables - I'm open for better options :)

[RincewindsHat]

Hi @gunzl1ng3r,
Thanks! I had a quick look and it looks good on a first glance, but @martialblog should probably get involved, since my knowledge of Prometheus stuff is very limited.

Just two things from me:

1.Please fix whatever the golang linter has found (it is kinda right most of the time). It the linter is "wrong" we can discuss it, but then the problematic parts must get exclusions from the linter runs
2.Would it be better to interpret the arguments to --groups as regex expressions, so people with groups like foo1, foo2 can just exclude them with a single parameter?

[martialblog]

Hi,

the feature makes sense, we can include it in a future version.

Regarding regex, since the --name parameter doesn't use regex right now, I think it's fine for now to have --group be plain strings as well. We can still add that for both at a later point, just so that it's consistent for now.

Thanks!

[martialblog]

This function is getting a bit complex, I wonder if we can simplify the logic a bit and avoid too much nesting.

[martialblog]

@gunzl1ng3r Here's some inspiration:

// Flattens a list of RuleGroup containing a list of Rules into
// a list of internal Alertingrules.
var l int

// Set initial capacity to reduce memory allocations.
for _, grp := range groups {
        l += len(grp.Rules)
}

// If we have wanted groups the capacity is at max
// the length of the wanted groups
if len(wantedGroups) > 0 {
        l = len(wantedGroups)
}

rules := make([]Rule, 0, l)

var r Rule

for _, grp := range groups {
        // If we have wanted groups we only care about
        // these groups, thus we can skip if not in wanted groups.
        if len(wantedGroups) > 0 {
                if !slices.Contains(wantedGroups, grp.Name) {
                        continue
                }
        }

        for _, rl := range grp.Rules {
                // For now we only care about AlertingRules,
                // since RecodingRules can simply be queried.
                if _, ok := rl.(v1.AlertingRule); ok {
                        r.AlertingRule = rl.(v1.AlertingRule)
                        rules = append(rules, r)
                }
        }
}

return rules

[gunzl1ng3r]

I know it's just an example, but this wouldn't work, would it? As is read it l should contain the total number of rules found in all groups. With this example it would contain the number of wantedGroups.

I pushed an update, that should gets rid of the else constructs.

[martialblog]

I agree that having the JSON responses in here is a bit much, but if we touch it, then the long-term solution would probably be a directory with JSON files.

Could you maybe split these changes into two PRs? So that this the feature gets the focus it deserves.

We already have a tests/ directory, I'd say we add a tests/testdata/ directory with JSON files for each test scenario.

ls tests/testdata/

empty-response.json
multiple-groups.json
single-alerts.json
...

[gunzl1ng3r]

yes, that would be the clean way - do you want the edit to be rolled back? I'd suggest leaving it in, as it does not change behaviour but makes the following change easier.