Alerting: Enable Unified Alerting for open source and enterprise #49834
Conversation
grobinson-grafana
requested review from
gotjosh,
santihernandezc,
zserge,
sh0rez and
idafurjes
and removed request for
a team
grobinson-grafana
requested review from
armandgrillet,
a team,
yuri-tceretian,
gotjosh,
JohnnyQQQQ and
joeblubaugh
and removed request for
gotjosh,
santihernandezc,
zserge,
sh0rez,
idafurjes,
yuri-tceretian and
armandgrillet
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/20233 |
This commit enables Unified Alerting for open source and enterprise unless disabled in configuration.
grobinson-grafana
force-pushed
the
grobinson/alerting-feature-ua-migrations-in-g9
branch
from
397c8b4 to
e0b7ef1
Compare
| // At present an invalid value is considered the same as no value. This means that a | ||
| // spelling mistake in the string "false" could enable unified alerting rather | ||
| // than disable it. This issue can be found here | ||
| unifiedAlerting, err := section.Key("enabled").Bool() |
There was a problem hiding this comment.
Should we take the opportunity to fix this?
I think:
section.Key("enabled").MustBool(false)
Should be a safe bet to disable UA if there's ambiguity in the value?
There was a problem hiding this comment.
I spoke to Armand and we can fix this in a follow up due to the release going out tomorrow. It might require more careful changes and tests.
| // At present an invalid value is considered the same as no value. This means that a | ||
| // spelling mistake in the string "false" could enable unified alerting rather | ||
| // than disable it. This issue can be found here | ||
| unifiedAlerting, err := section.Key("enabled").Bool() | ||
| if err != nil { | ||
| // TODO: Remove in Grafana v9 |
There was a problem hiding this comment.
If we implement the above, I assume that's what this comment is referring to and we no longer need this block.
There was a problem hiding this comment.
The block would still be present but would change the if condition to section.HasKey("enabled"). There would be a separate if statement for when section.Key("enabled").Bool() returns an error.
| } | ||
| // NOTE: If the enabled flag is still not defined, the final decision is made during migration (see sqlstore.migrations.ualert.CheckUnifiedAlertingEnabledByDefault). | ||
| cfg.Logger.Info("The state of unified alerting is still not defined. The decision will be made during as we run the database migrations") |
There was a problem hiding this comment.
Isn't this partially true? IIRC were introduced a safety check to avoid losing data so I think there might be a case where a decision can be made at migration time - can you please double check?
There was a problem hiding this comment.
I understand that this was required as before this change we had the following:
| Alerting | Unified Alerting | alerts table has alerts | Outcome |
|---|---|---|---|
| true | "" | true | Legacy |
| true | "" | false | UA |
| "" | "" | true | Legacy |
| "" | "" | false | UA |
but now we changed this table to:
| Alerting | Unified Alerting | alerts table has alerts | Outcome |
|---|---|---|---|
| true | "" | true | UA |
| true | "" | false | UA |
| "" | "" | true | UA |
| "" | "" | false | UA |
i.e. whether or not the database has alerts is irrelevant to the outcome and there is no longer a decision to be made at migration time.
grobinson-grafana
deleted the
grobinson/alerting-feature-ua-migrations-in-g9
branch
) This commit enables Unified Alerting for open source and enterprise unless disabled in configuration. (cherry picked from commit 3b7f871)
) (#49845) This commit enables Unified Alerting for open source and enterprise unless disabled in configuration. (cherry picked from commit 3b7f871) Co-authored-by: George Robinson <[email protected]>
|
@grobinson-grafana can you please add a changelog entry? sorry for not mentioning it early - I completely forgot 🙈 |
|
@gotjosh : Does this PR allows to manage alerts as code with sidecars, like we used to operated using 8.2.7 release of Grafana OpenSource edition ? Unified Alert was a breaking change for this use case, it would be really great to recover this value and take the full benefit of "as code" way of working |
|
@cbernard-psee not for now, this is tracked in #36153, the API for it is part of 9.0 but we didn't implement any provider yet (File provisioning, Terraform, etc.). Those providers are planned for 9.1 and 9.2. |
What this PR does / why we need it:
This pull request enables unified alerting for both open source and enterprise Grafana unless disabled in configuration.
It is enabled in the following situations:
The following table shows the outcomes of different settings: