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

notification: remove DestType and Dest #4022

Merged
merged 21 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
fb299af
Refactor to use string for destination type
mastercactapus Jul 30, 2024
551922d
Refactored contact method activation logic
mastercactapus Jul 30, 2024
33ee206
Refactor to use gadb.DestV1 for destinations
mastercactapus Jul 30, 2024
1d7afc4
Refactor notification handling and database queries
mastercactapus Jul 30, 2024
c3a728b
Fix notification status lookup with correct destination ID
mastercactapus Jul 30, 2024
7d4a3b4
Refactor notification destination types
mastercactapus Jul 30, 2024
1f09610
Refactor message destination field access
mastercactapus Jul 30, 2024
29218f1
Refactor notification destination handling
mastercactapus Jul 30, 2024
d9fc84b
Refactor notification handling for consistency
mastercactapus Jul 30, 2024
01d1fd3
Update rate limit logic for Twilio and email notifications
mastercactapus Jul 30, 2024
65f686b
Merge branch 'master' into msg-dest-v1
mastercactapus Jul 30, 2024
f2ee4c1
return blank for unknown
mastercactapus Jul 30, 2024
43bb1c3
Refactor to use simplified destination type
mastercactapus Jul 30, 2024
57ac39f
Refactor message destination handling
mastercactapus Jul 30, 2024
ce637e7
Change ProviderType from json.Number to string in logs
mastercactapus Jul 30, 2024
1a3f486
Remove redundant code in notification package
mastercactapus Jul 30, 2024
bb7f405
pr size
mastercactapus Jul 30, 2024
27b47df
pr size
mastercactapus Jul 30, 2024
992e2f5
pr size
mastercactapus Jul 30, 2024
0cbb9e7
Refactor destination handling for alerts
mastercactapus Jul 31, 2024
fde01ba
Merge branch 'master' into rm-notif-desttype
mastercactapus Jul 31, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions devtools/resetdb/datagen.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"github.com/target/goalert/heartbeat"
"github.com/target/goalert/integrationkey"
"github.com/target/goalert/label"
"github.com/target/goalert/notification"
"github.com/target/goalert/override"
"github.com/target/goalert/permission"
"github.com/target/goalert/schedule"
Expand Down Expand Up @@ -159,7 +158,7 @@ func (d *datagen) NewCM(userID string) {
cm.Type = contactmethod.TypeVoice
}

cm.Value = d.ids.Gen(d.genPhone, notification.ScannableDestType{CM: cm.Type}.DestType().String())
cm.Value = d.ids.Gen(d.genPhone, string(cm.Type))
d.ContactMethods = append(d.ContactMethods, cm)
}

Expand Down
4 changes: 2 additions & 2 deletions engine/message/bundlealerts.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ func bundleAlertMessages(messages []Message, newBundleFunc func(Message) (string
})

type key struct {
notification.DestHash
notification.DestID
ServiceID string
}

groups := make(map[key][]Message)
for _, msg := range toProcess {
key := key{
DestHash: msg.Dest.DestHash(),
DestID: msg.DestID,
ServiceID: msg.ServiceID,
}
groups[key] = append(groups[key], msg)
Expand Down
18 changes: 5 additions & 13 deletions engine/message/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,9 @@ func (db *DB) currentQueue(ctx context.Context, tx *sql.Tx, now time.Time) (*que
}
msg.CreatedAt = row.CreatedAt
msg.SentAt = row.SentAt.Time
msg.Dest.ID.CMID = row.CmID
msg.Dest.ID.NCID = row.ChanID
msg.Dest.DestV1 = row.Dest.DestV1
msg.DestID.CMID = row.CmID
msg.DestID.NCID = row.ChanID
msg.Dest = row.Dest.DestV1
msg.StatusAlertIDs = row.StatusAlertIds
if row.ScheduleID.Valid {
msg.ScheduleID = row.ScheduleID.UUID.String()
Expand Down Expand Up @@ -398,22 +398,14 @@ func (db *DB) currentQueue(ctx context.Context, tx *sql.Tx, now time.Time) (*que
}

result, err = bundleAlertMessages(result, func(msg Message) (string, error) {
var cmID, chanID uuid.NullUUID
var userID sql.NullString
if msg.UserID != "" {
userID.Valid = true
userID.String = msg.UserID
}
if msg.Dest.ID.IsUserCM() {
cmID.Valid = true
cmID.UUID = msg.Dest.ID.UUID()
} else {
chanID.Valid = true
chanID.UUID = msg.Dest.ID.UUID()
}

newID := uuid.NewString()
_, err := tx.StmtContext(ctx, db.createAlertBundle).ExecContext(ctx, newID, msg.CreatedAt, cmID, chanID, userID, msg.ServiceID)
_, err := tx.StmtContext(ctx, db.createAlertBundle).ExecContext(ctx, newID, msg.CreatedAt, msg.DestID.CMID, msg.DestID.NCID, userID, msg.ServiceID)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -778,7 +770,7 @@ func (db *DB) sendMessagesByType(ctx context.Context, cLock *processinglock.Conn

func (db *DB) sendMessage(ctx context.Context, cLock *processinglock.Conn, send SendFunc, m *Message) (bool, error) {
ctx = log.WithFields(ctx, log.Fields{
"DestTypeID": m.Dest.ID,
"DestID": m.DestID,
"DestType": m.Dest.Type,
"CallbackID": m.ID,
})
Expand Down
4 changes: 2 additions & 2 deletions engine/message/dedupalerts.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@ func dedupAlerts(msgs []Message, bundleFunc func(parentID string, duplicateIDs [
sort.Slice(toProcess, func(i, j int) bool { return toProcess[i].CreatedAt.Before(toProcess[j].CreatedAt) })

type msgKey struct {
notification.DestHash
notification.DestID
AlertID int
}
alerts := make(map[msgKey]string, len(msgs))
duplicates := make(map[string][]string)

for _, msg := range toProcess {
// check if we have seen this alert before
key := msgKey{msg.Dest.DestHash(), msg.AlertID}
key := msgKey{msg.DestID, msg.AlertID}

if parentID, ok := alerts[key]; ok {
duplicates[parentID] = append(duplicates[parentID], msg.ID)
Expand Down
20 changes: 10 additions & 10 deletions engine/message/dedupalerts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ func TestDedupAlerts(t *testing.T) {
messages := []Message{
{ID: "1", Type: notification.MessageTypeTest},
{ID: "2", Type: notification.MessageTypeAlertBundle},
{ID: "3", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 11, 0, 0, 0, time.UTC), AlertID: 1, Dest: notification.Dest{ID: foo}}, // duplicates 4 (same dest and alert, but newer)
{ID: "4", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 10, 0, 0, 0, time.UTC), AlertID: 1, Dest: notification.Dest{ID: foo}},
{ID: "5", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 12, 0, 0, 0, time.UTC), AlertID: 1, Dest: notification.Dest{ID: foo}}, // duplicates 4 (same dest and alert, but newer)
{ID: "3", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 11, 0, 0, 0, time.UTC), AlertID: 1, DestID: foo}, // duplicates 4 (same dest and alert, but newer)
{ID: "4", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 10, 0, 0, 0, time.UTC), AlertID: 1, DestID: foo},
{ID: "5", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 12, 0, 0, 0, time.UTC), AlertID: 1, DestID: foo}, // duplicates 4 (same dest and alert, but newer)

{ID: "6", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 8, 0, 0, 0, time.UTC), AlertID: 1, Dest: notification.Dest{ID: bar}, SentAt: time.Unix(1, 0)}, // duplicates 7 but sent already
{ID: "6", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 8, 0, 0, 0, time.UTC), AlertID: 1, DestID: bar, SentAt: time.Unix(1, 0)}, // duplicates 7 but sent already

{ID: "7", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 9, 0, 0, 0, time.UTC), AlertID: 1, Dest: notification.Dest{ID: bar}},
{ID: "8", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 13, 0, 0, 0, time.UTC), AlertID: 2, Dest: notification.Dest{ID: foo}},
{ID: "7", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 9, 0, 0, 0, time.UTC), AlertID: 1, DestID: bar},
{ID: "8", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 13, 0, 0, 0, time.UTC), AlertID: 2, DestID: foo},
}

res, err := dedupAlerts(messages, func(parentID string, duplicates []string) error {
Expand All @@ -43,10 +43,10 @@ func TestDedupAlerts(t *testing.T) {
[]Message{
{ID: "1", Type: notification.MessageTypeTest},
{ID: "2", Type: notification.MessageTypeAlertBundle},
{ID: "4", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 10, 0, 0, 0, time.UTC), AlertID: 1, Dest: notification.Dest{ID: foo}},
{ID: "7", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 9, 0, 0, 0, time.UTC), AlertID: 1, Dest: notification.Dest{ID: bar}},
{ID: "8", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 13, 0, 0, 0, time.UTC), AlertID: 2, Dest: notification.Dest{ID: foo}},
{ID: "6", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 8, 0, 0, 0, time.UTC), AlertID: 1, Dest: notification.Dest{ID: bar}, SentAt: time.Unix(1, 0)},
{ID: "4", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 10, 0, 0, 0, time.UTC), AlertID: 1, DestID: foo},
{ID: "7", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 9, 0, 0, 0, time.UTC), AlertID: 1, DestID: bar},
{ID: "8", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 13, 0, 0, 0, time.UTC), AlertID: 2, DestID: foo},
{ID: "6", Type: notification.MessageTypeAlert, CreatedAt: time.Date(2021, 7, 15, 8, 0, 0, 0, time.UTC), AlertID: 1, DestID: bar, SentAt: time.Unix(1, 0)},
},
res)
}
4 changes: 2 additions & 2 deletions engine/message/deduponcallnotifications.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ func dedupOnCallNotifications(messages []Message) ([]Message, []string) {

type msgKey struct {
scheduleID string
dest notification.DestHash
dest notification.DestID
}

m := make(map[msgKey]struct{})
var toDelete []string
for _, msg := range toProcess {
key := msgKey{scheduleID: msg.ScheduleID, dest: msg.Dest.DestHash()}
key := msgKey{scheduleID: msg.ScheduleID, dest: msg.DestID}
if _, ok := m[key]; ok {
toDelete = append(toDelete, msg.ID)
continue
Expand Down
4 changes: 2 additions & 2 deletions engine/message/dedupstatusmessages.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ func dedupStatusMessages(messages []Message) ([]Message, []string) {

type msgKey struct {
alertID int
dest notification.DestHash
dest notification.DestID
}

m := make(map[msgKey]struct{})
var toDelete []string
for _, msg := range toProcess {
key := msgKey{alertID: msg.AlertID, dest: msg.Dest.DestHash()}
key := msgKey{alertID: msg.AlertID, dest: msg.DestID}
if _, ok := m[key]; ok {
toDelete = append(toDelete, msg.ID)
continue
Expand Down
4 changes: 3 additions & 1 deletion engine/message/message.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@ package message
import (
"time"

"github.com/target/goalert/gadb"
"github.com/target/goalert/notification"
)

// Message represents the data for an outgoing message.
type Message struct {
ID string
Type notification.MessageType
Dest notification.Dest
DestID notification.DestID
Dest gadb.DestV1
AlertID int
AlertLogID int
VerifyID string
Expand Down
4 changes: 2 additions & 2 deletions engine/message/queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ func (q *queue) addSent(m Message) {
if t := q.userSent[m.UserID]; m.SentAt.After(t) {
q.userSent[m.UserID] = m.SentAt
}
if t := q.destSent[m.Dest.ID]; m.SentAt.After(t) {
q.destSent[m.Dest.ID] = m.SentAt
if t := q.destSent[m.DestID]; m.SentAt.After(t) {
q.destSent[m.DestID] = m.SentAt
}

q.sent = append(q.sent, m)
Expand Down
12 changes: 6 additions & 6 deletions engine/message/queue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@ import (
"github.com/target/goalert/notification/twilio"
)

func voice(value string) notification.Dest {
return notification.Dest{DestV1: gadb.NewDestV1(twilio.DestTypeTwilioVoice, twilio.FieldPhoneNumber, value)}
func voice(value string) gadb.DestV1 {
return gadb.NewDestV1(twilio.DestTypeTwilioVoice, twilio.FieldPhoneNumber, value)
}

func sms(value string) notification.Dest {
return notification.Dest{DestV1: gadb.NewDestV1(twilio.DestTypeTwilioSMS, twilio.FieldPhoneNumber, value)}
func sms(value string) gadb.DestV1 {
return gadb.NewDestV1(twilio.DestTypeTwilioSMS, twilio.FieldPhoneNumber, value)
}

func slackCh(value string) notification.Dest {
return notification.Dest{DestV1: gadb.NewDestV1(slack.DestTypeSlackChannel, slack.FieldSlackChannelID, value)}
func slackCh(value string) gadb.DestV1 {
return gadb.NewDestV1(slack.DestTypeSlackChannel, slack.FieldSlackChannelID, value)
}

func TestQueue_Sort(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions engine/message/ratelimit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ func TestRateLimit(t *testing.T) {
last := times[len(times)-1]
th := message.NewThrottle(message.PerCMThrottle, last, false)
for _, tm := range times[:len(times)-1] {
th.Record(message.Message{Type: msgType, SentAt: tm, Dest: notification.Dest{DestV1: gadb.DestV1{Type: destType}}})
th.Record(message.Message{Type: msgType, SentAt: tm, Dest: gadb.DestV1{Type: destType}})
}
assert.Falsef(t, th.InCooldown(message.Message{Type: msgType, Dest: notification.Dest{DestV1: gadb.DestV1{Type: destType}}}), "message #%d should not be in cooldown", i)
assert.Falsef(t, th.InCooldown(message.Message{Type: msgType, Dest: gadb.DestV1{Type: destType}}), "message #%d should not be in cooldown", i)
}
})
}
Expand Down
10 changes: 5 additions & 5 deletions engine/message/throttle.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"crypto/sha256"
"time"

"github.com/target/goalert/notification"
"github.com/target/goalert/gadb"
)

// Throttle represents the throttled messages for a queue.
Expand All @@ -15,12 +15,12 @@ type Throttle struct {

first map[ThrottleItem]time.Time
count map[ThrottleItem]int
cooldown map[notification.DestHash]bool
cooldown map[gadb.DestHashV1]bool
}

// ThrottleItem represents the messages being throttled.
type ThrottleItem struct {
DestHash notification.DestHash
DestHash gadb.DestHashV1
BucketDur time.Duration
}

Expand Down Expand Up @@ -50,11 +50,11 @@ func NewThrottle(cfg ThrottleConfig, now time.Time, byTypeOnly bool) *Throttle {

first: make(map[ThrottleItem]time.Time),
count: make(map[ThrottleItem]int),
cooldown: make(map[notification.DestHash]bool),
cooldown: make(map[gadb.DestHashV1]bool),
}
}

func (tr *Throttle) destKey(d notification.Dest) notification.DestHash {
func (tr *Throttle) destKey(d gadb.DestV1) gadb.DestHashV1 {
if tr.typeOnly {
return sha256.Sum256([]byte(d.Type))
}
Expand Down
2 changes: 1 addition & 1 deletion engine/message/throttleconfigbuilder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestThrottleConfigBuilder(t *testing.T) {

check := func(dest string, msg notification.MessageType, expRules []message.ThrottleRule) {
t.Helper()
assert.EqualValues(t, expRules, cfg.Rules(message.Message{Type: msg, Dest: notification.Dest{DestV1: gadb.DestV1{Type: dest}}}))
assert.EqualValues(t, expRules, cfg.Rules(message.Message{Type: msg, Dest: gadb.DestV1{Type: dest}}))
}

check(
Expand Down
33 changes: 12 additions & 21 deletions engine/sendmessage.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package engine

import (
"context"
"database/sql"
"encoding/json"
"fmt"

Expand All @@ -19,16 +18,16 @@ import (
func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notification.SendResult, error) {
ctx = log.WithField(ctx, "CallbackID", msg.ID)

if msg.Dest.ID.IsUserCM() {
if msg.DestID.IsUserCM() {
ctx = permission.UserSourceContext(ctx, msg.UserID, permission.RoleUser, &permission.SourceInfo{
Type: permission.SourceTypeContactMethod,
ID: msg.Dest.ID.UUID().String(),
ID: msg.DestID.String(),
})
} else {
ctx = permission.SystemContext(ctx, "SendMessage")
ctx = permission.SourceContext(ctx, &permission.SourceInfo{
Type: permission.SourceTypeNotificationChannel,
ID: msg.Dest.ID.UUID().String(),
ID: msg.DestID.String(),
})
}

Expand All @@ -51,7 +50,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
}, nil
}
notifMsg = notification.AlertBundle{
Dest: msg.Dest.DestV1,
Dest: msg.Dest,
CallbackID: msg.ID,
ServiceID: msg.ServiceID,
ServiceName: name,
Expand All @@ -66,7 +65,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
if err != nil {
return nil, errors.Wrap(err, "lookup alert")
}
stat, err := p.cfg.NotificationStore.OriginalMessageStatus(ctx, msg.AlertID, msg.Dest.ID)
stat, err := p.cfg.NotificationStore.OriginalMessageStatus(ctx, msg.AlertID, msg.DestID)
if err != nil {
return nil, fmt.Errorf("lookup original message: %w", err)
}
Expand Down Expand Up @@ -100,7 +99,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
if err != nil {
return nil, fmt.Errorf("lookup original alert: %w", err)
}
stat, err := p.cfg.NotificationStore.OriginalMessageStatus(ctx, msg.AlertID, msg.Dest.ID)
stat, err := p.cfg.NotificationStore.OriginalMessageStatus(ctx, msg.AlertID, msg.DestID)
if err != nil {
return nil, fmt.Errorf("lookup original message: %w", err)
}
Expand All @@ -119,7 +118,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
}

notifMsg = notification.AlertStatus{
Dest: msg.Dest.DestV1,
Dest: msg.Dest,
AlertID: e.AlertID(),
ServiceID: a.ServiceID,
CallbackID: msg.ID,
Expand All @@ -131,7 +130,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
}
case notification.MessageTypeTest:
notifMsg = notification.Test{
Dest: msg.Dest.DestV1,
Dest: msg.Dest,
CallbackID: msg.ID,
}
case notification.MessageTypeVerification:
Expand All @@ -140,7 +139,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
return nil, errors.Wrap(err, "lookup verification code")
}
notifMsg = notification.Verification{
Dest: msg.Dest.DestV1,
Dest: msg.Dest,
CallbackID: msg.ID,
Code: code,
}
Expand All @@ -164,7 +163,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
}

notifMsg = notification.ScheduleOnCallUsers{
Dest: msg.Dest.DestV1,
Dest: msg.Dest,
CallbackID: msg.ID,
ScheduleName: sched.Name,
ScheduleURL: p.cfg.ConfigSource.Config().CallbackURL("/schedules/" + msg.ScheduleID),
Expand All @@ -188,7 +187,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
}

notifMsg = notification.SignalMessage{
Dest: msg.Dest.DestV1,
Dest: msg.Dest,
CallbackID: msg.ID,
Params: params,
}
Expand Down Expand Up @@ -217,15 +216,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi
}

if isFirstAlertMessage && res.State.IsOK() {
var chanID, cmID sql.NullString
if msg.Dest.ID.IsUserCM() {
cmID.Valid = true
cmID.String = msg.Dest.ID.String()
} else {
chanID.Valid = true
chanID.String = msg.Dest.ID.String()
}
_, err = p.b.trackStatus.ExecContext(ctx, chanID, cmID, msg.AlertID)
_, err = p.b.trackStatus.ExecContext(ctx, msg.DestID.NCID, msg.DestID.CMID, msg.AlertID)
if err != nil {
// non-fatal, but log because it means status updates will not work for that alert/dest.
log.Log(ctx, fmt.Errorf("track status updates for alert #%d for %s: %w", msg.AlertID, msg.Dest.String(), err))
Expand Down
Loading
Loading