Alerting: In migration improve deduplication of title and group #78351
Conversation
Now when duplicate titles are detected we will first attempt to use the dashboard and panel ids to deduplicate, falling back to a random uid. Groups are no longer deduplicated, instead we set them to a combination of truncated dashboard name and alert frequency. This way, alerts from the same dashboard share a group if they have the same evaluation interval. In the event that truncation causes overlap, it won't be a big issue as all alerts will still be in a group with the correct evaluation interval.
JacobsonMT
requested review from
rwwiv,
yuri-tceretian and
grobinson-grafana
and removed request for
a team
JacobsonMT
added
area/alerting
Improves Deduplicator to encapsulate more of the logic
JacobsonMT
added
the
area/alerting/migration
|
|
||
| // None of the simple suffixes worked, so we generate a new uid. We try a few times, just in case but this should | ||
| // almost always create a unique string on the first try. | ||
| for i := 0; i < 5; i++ { |
There was a problem hiding this comment.
Perhaps we can reuse constant MaxIncrementDeduplicationAttempts here by slightly renaming it. Not a blocker at all. Just for the sake of not using magic literals in code :)
| } | ||
|
|
||
| func Test_shortUIDCaseInsensitiveConflicts(t *testing.T) { | ||
| s := Deduplicator{ |
There was a problem hiding this comment.
I wonder if it can be simplified to just two lines with predefined UIDs like:
- "abc"
- "Abc"
and then just count number of keys when caseInsensitive is true and false
There was a problem hiding this comment.
Honestly, this test doesn't really make much sense anymore since we're now using uuid.NewRandom. It was testing issues with the algorithm when creating uids in quick succession. I'd rather leave it as is, or remove it entirely.
| @@ -61,8 +61,6 @@ type Store interface { | |||
| SetOrgMigrationState(ctx context.Context, orgID int64, summary *migmodels.OrgMigrationState) error | |||
|
|
|||
| RevertAllOrgs(ctx context.Context) error | |||
|
|
|||
| CaseInsensitive() bool | |||
There was a problem hiding this comment.
Personally, I think this is a good thing.
| silences: make([]*pb.MeshSilence, 0), | ||
| titleDeduplicatorForFolder: func(folderUID string) *migmodels.Deduplicator { | ||
| if _, ok := titlededuplicatorPerFolder[folderUID]; !ok { | ||
| titlededuplicatorPerFolder[folderUID] = migmodels.NewDeduplicator(ms.store.GetDialect().SupportEngine(), store.AlertDefinitionMaxTitleLength) |
There was a problem hiding this comment.
I think ms.migrationStore.CaseInsensitive() was better because it hid the store's implementation details
There was a problem hiding this comment.
I'll add it back, I don't feel strongly here. Removed it because this was the only place it's used and we have access to the base store.
* Alerting: In migration improve deduplication of title and group This change improves alert titles generated in the legacy migration that occur when we need to deduplicate titles. Now when duplicate titles are detected we will first attempt to append a sequential index, falling back to a random uid if none are unique within 10 attempts. This should cause shorter and more easily readable deduplicated titles in most cases. In addition, groups are no longer deduplicated. Instead we set them to a combination of truncated dashboard name and humanized alert frequency. This way, alerts from the same dashboard share a group if they have the same evaluation interval. In the event that truncation causes overlap, it won't be a big issue as all alerts will still be in a group with the correct evaluation interval.
What is this feature?
This change improves alert titles generated in the legacy migration that occur when we need to deduplicate titles. Now when duplicate titles are detected we will first attempt to append a sequential index, falling back to a random uid if none are unique within 10 attempts. This should cause shorter and more easily readable deduplicated titles in most cases.
In addition, groups are no longer deduplicated. Instead we set them to a combination of truncated dashboard name and humanized alert frequency. This way, alerts from the same dashboard share a group if they have the same evaluation interval. In the event that truncation causes overlap, it won't be a big issue as all alerts will still be in a group with the correct evaluation interval.
Who is this feature for?
Users migrating from legacy alerting to Grafana Alerting (UA)
alertAinpanel1indashboard1infolder1-60sintervalalertAinpanel2indashboard1infolder1-60sintervalalertBinpanel1indashboard2infolder1-300sintervalThese would migrate to as follows.
Before this change (example uid):
alertAin groupdashboard1 - 1infolder1alertA_c976a621-4f7a-4963-82c3-0e58e2e5311fin groupdashboard1 - 2infolder1alertBin groupdashboard2 - 1infolder1After this change:
alertAin groupdashboard1 - 1minfolder1alertA #2in groupdashboard1 - 1minfolder1alertBin groupdashboard2 - 5minfolder1