feat: alert_for - #788
feat: alert_for#788robbat2 wants to merge 1 commit into
Conversation
a416959 to
2ea0968
Compare
Provides alert tuning of `for:` Prometheus clauses. Note that it requires a tiny addition to the API structure to represent the new field. Reference: slok#787 Signed-off-by: Robin H. Johnson <rjohnson@coreweave.com>
2ea0968 to
1c086b0
Compare
|
@slok could you please review & merge? |
|
@slok bump |
|
+1 from me this plugin seems awesome |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days. |
cxdy
left a comment
There was a problem hiding this comment.
Hey @robbat2, thank you for your contribution and sorry for the delay!
Some nits/suggestions, only really blocking comment is the --slo-plugins-path being invalid.
And just want to clarify, you only want to support the prometheus/v1 spec, right? If so, please make that super clear in comments/docs. I reviewed this as if you were also trying to be able to use this with Kubernetes/OpenSLO specs.
|
|
||
| ```bash | ||
| sloth generate -i ./examples/getting-started.yml | ||
| sloth generate --slo-plugins-path=./examples/plugins -i ./examples/getting-started.yml |
There was a problem hiding this comment.
--slo-plugins-path was renamed in v0.13.0 - https://github.com/slok/sloth/releases#release-v0.13.0
Use --plugins-path / -p here instead.
| // Default is 0m, which means no pending time. | ||
| // | ||
| // Sloth core plugins ignore this field. Use an output plugin (SLO plugin) that applies | ||
| // it on the generated Prometheus rules, like the `custom_alert_for.go` example. |
There was a problem hiding this comment.
I'd probably change this comment to something like
// Sloth core plugins ignore this field. Enable the contrib SLO plugin
// sloth.dev/contrib/alert_for/v1 on slo_plugins.chain to apply it to
// the generated Prometheus rules.
"the custom_alert_for.go example" is confusing, as it doesn't seem to exist?
|
|
||
| type plugin struct{} | ||
|
|
||
| func (p plugin) ProcessSLO(_ context.Context, request *pluginslov1.Request, result *pluginslov1.Result) error { |
There was a problem hiding this comment.
ProcessSLO returns nil when OriginalSource.SlothV1 is nil, the SLO name is missing from that spec, or both durations are zero. Kubernetes controller specs (OriginalSource.K8sSlothV1 only), OpenSLO, and library callers that build PromSLO without OriginalSource will seem successful but never apply for.
| PluginID = "sloth.dev/contrib/alert_for/v1" | ||
| ) | ||
|
|
||
| func NewPlugin(_ json.RawMessage, _ pluginslov1.AppUtils) (pluginslov1.Plugin, error) { |
There was a problem hiding this comment.
NewPlugin discards config JSON, so there is no supported way to set durations except the prometheus/v1 spec field.
The rule_intervals plugin keeps the config for example: https://github.com/slok/sloth/blob/main/internal/plugin/slo/contrib/rule_intervals_v1/plugin.go#L29
There was a problem hiding this comment.
If the target is only intended for the prometheus/v1 spec (i.e you don't want to support Kubernetes/OpenSLO), please make that abundantly clear in comments and documentation.
| owner: "myteam" | ||
| repo: "myorg/myservice" | ||
| tier: "2" | ||
| slo_plugins: |
There was a problem hiding this comment.
This isn't the right place for plugin examples, this is for the default config (without plugins).
Other contrib plugins live in examples/contrib-slo-plugins.yml, not getting-started. Committed examples/_gen/getting-started.yml was not regenerated, so it still has no for: even though README.md:102 presents that file as the result of this spec.
Keep getting-started free of contrib plugins. Add an SLO (or file) under examples/contrib-slo-plugins.yml and regenerate examples/_gen/ with make examples-gen.
| - `slos[].alerting.page_alert.for` | ||
| - `slos[].alerting.ticket_alert.for` | ||
|
|
||
| This plugin is required because the core plugins ignore `for` and always generate alerts without a pending time. |
There was a problem hiding this comment.
Other contrib plugin READMEs document Config, Env vars, and order requirement. This one does not.
Order matters: alert_rules_v1 assigns result.SLORules.AlertRules.Rules = rules, wiping any earlier For. Default priority 0 is placed in postDefault (priority < 0 is pre-default), so the README example works only if users do not give this plugin a negative priority or overridePrevious without alert_rules. Matching is by sloth_severity (page/ticket) from PageQuick/TicketQuick, which is correct for the single combined MWMB rule per severity, but is not described. Match the contrib README template. State that the plugin must run after sloth.dev/core/alert_rules/v1, that it is embedded, that it only reads prometheus/v1 OriginalSource, and that both quick and slow windows share one for per severity.
| res pluginslov1.Result | ||
| expRes pluginslov1.Result | ||
| }{ | ||
| "Using the plugin as embedded yaegi plugin, it should set page and ticket `for` durations.": { |
There was a problem hiding this comment.
The table has two copies of the same happy path (Yaegi vs compiled NewPlugin) and no other cases.
| }, | ||
| } | ||
|
|
||
| for name, test := range tests { |
There was a problem hiding this comment.
Ideas for other tests you could do:
SlothV1 == nil(K8s/OpenSLO no-op)- unmatched SLO name
- only page or only ticket
For - both durations zero
Labels == nil- rules that lack
sloth_severity - empty
MWMBAlertGroup(severity"unknown").
Keep one Yaegi process test (project pattern in info_labels_v1) and add no-op / partial-For cases. Optionally add a spec-loader case with for: 5m so the API field cannot regress.
| }, | ||
| }, | ||
|
|
||
| "Using the plugin as compiled Go plugin, it should set page and ticket `for` durations.": { |
There was a problem hiding this comment.
Nit: The compiled-Go case duplicates the entire Request/Result fixture from the Yaegi case. Other contrib plugins test Yaegi in the table and compiled NewPlugin in a benchmark (BenchmarkPluginGo). Share fixtures or drop the second table row in favor of a small compiled-path benchmark.
| // Default is 0m, which means no pending time. | ||
| // | ||
| // Sloth core plugins ignore this field. Use an output plugin (SLO plugin) that applies | ||
| // it on the generated Prometheus rules, like the `custom_alert_for.go` example. |
There was a problem hiding this comment.
Where is custom_alert_for.go? Could you update this comment please?
Provides alert tuning of
for:Prometheus clauses.Note that it requires a tiny addition to the API structure to represent the new field.
Reference: #787