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.

Sign up for GitHub

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

Write schedule/group_id for contact notified histories #64

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Write schedule/group_id for contact notified histories #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd94e4b
Add `recipient.Key#CopyNonNil()` method
yhabteab May 10, 2023
272efd5
Write also `schedule/contactgroup_id` for contact notified histories
yhabteab May 10, 2023
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
67 changes: 57 additions & 10 deletions internal/listener/listener.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,9 @@ func (l *Listener) ProcessEvent(w http.ResponseWriter, req *http.Request) {
contactChannels := make(map[*recipient.Contact]map[string]struct{})

escalationRecipients := make(map[recipient.Key]bool)
groupsOrSchedules := make(map[*recipient.Contact]recipient.Key)
groupsOrSchedulesWithoutMembers := make(map[recipient.Recipient]bool)

for escalationID, state := range currentIncident.EscalationState {
escalation := l.runtimeConfig.GetRuleEscalation(escalationID)
if state.TriggeredAt.Time().IsZero() {
Expand Down Expand Up @@ -447,7 +450,14 @@ func (l *Listener) ProcessEvent(w http.ResponseWriter, req *http.Request) {
escalationRecipients[escalationRecipient.Key] = true

if !managed || state.Role > incident.RoleRecipient {
for _, c := range escalationRecipient.Recipient.GetContactsAt(ev.Time) {
r := escalationRecipient.Recipient
_, isContact := r.(*recipient.Contact)
contacts := r.GetContactsAt(ev.Time)
for _, c := range contacts {
if !isContact {
groupsOrSchedules[c] = recipient.ToKey(r)
}
Comment on lines +457 to +459
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's supposed to happen if the same contact if references multiple times, i.e. directly, via a group and via a schedule? This would just arbitrarily pick a group or schedule. We could write all of that information into the database, but that would probably mean writing multiple rows for the same notification, i.e. the web interface would have to handle and display this probably, so I don't want to decide this on my own.

No matter how we do it, a comment in the schema file how these rows are to be interpreted would be great.


if contactChannels[c] == nil {
contactChannels[c] = make(map[string]struct{})
}
Expand All @@ -457,6 +467,10 @@ func (l *Listener) ProcessEvent(w http.ResponseWriter, req *http.Request) {
contactChannels[c][c.DefaultChannel] = struct{}{}
}
}

if !isContact && len(contacts) == 0 {
groupsOrSchedulesWithoutMembers[r] = true
}
}
}
}
Expand All @@ -469,25 +483,43 @@ func (l *Listener) ProcessEvent(w http.ResponseWriter, req *http.Request) {

isEscalationRecipient := escalationRecipients[recipientKey]
if !isEscalationRecipient && (!managed || state.Role > incident.RoleRecipient) {
for _, contact := range r.GetContactsAt(ev.Time) {
_, isContact := r.(*recipient.Contact)
contacts := r.GetContactsAt(ev.Time)
for _, contact := range contacts {
if !isContact {
groupsOrSchedules[contact] = recipient.ToKey(r)
}

if contactChannels[contact] == nil {
contactChannels[contact] = make(map[string]struct{})
}
contactChannels[contact][contact.DefaultChannel] = struct{}{}
}

if !isContact && len(contacts) == 0 {
groupsOrSchedulesWithoutMembers[r] = true
}
}
}

for contact, channels := range contactChannels {
rk := recipient.ToKey(contact)
hr := &incident.HistoryRow{
EventID: utils.ToDBInt(ev.ID),
Time: types.UnixMilli(ev.Time),
Type: incident.Notified,
CausedByIncidentHistoryID: causedByIncidentHistoryId,
}

groupOrSchedule := groupsOrSchedules[contact]
if groupsOrSchedules != nil {
rk = rk.CopyNonNil(groupOrSchedule)
}

hr.Key = rk

for chType := range channels {
hr := &incident.HistoryRow{
Key: recipient.ToKey(contact),
EventID: utils.ToDBInt(ev.ID),
Time: types.UnixMilli(time.Now()),
Type: incident.Notified,
ChannelType: utils.ToDBString(chType),
CausedByIncidentHistoryID: causedByIncidentHistoryId,
}
hr.ChannelType = utils.ToDBString(chType)

l.logger.Infof("[%s %s] notify %q via %q", obj.DisplayName(), currentIncident.String(), contact.FullName, chType)

Expand Down Expand Up @@ -516,6 +548,21 @@ func (l *Listener) ProcessEvent(w http.ResponseWriter, req *http.Request) {
}
}

for gs, _ := range groupsOrSchedulesWithoutMembers {
hr := &incident.HistoryRow{
Key: recipient.ToKey(gs),
EventID: utils.ToDBInt(ev.ID),
Time: types.UnixMilli(ev.Time),
Type: incident.Notified,
CausedByIncidentHistoryID: causedByIncidentHistoryId,
}

_, err = currentIncident.AddHistory(hr, false)
if err != nil {
l.logger.Errorln(err)
}
}

_, _ = fmt.Fprintln(w)
}

Expand Down
18 changes: 18 additions & 0 deletions internal/recipient/recipient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,24 @@ func (r Key) MarshalText() (text []byte, err error) {
}
}

// CopyNonNil copies non-nil fields from the provided recipient.Key to the current one and
// returns the modified recipient key.
func (r Key) CopyNonNil(recipientKey Key) Key {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function and how you use recipient.Key in incident.HistoryRow contradicts the current assumption in the code that there's a 1:1 relation between recipient.Recipient and recipient.Key, as it can be seen in multiple places:

icinga-notifications/internal/recipient/recipient.go

Lines 16 to 21 in 002d14d

type Key struct {
// Only one of the members is allowed to be a non-zero value.
ContactID types.Int `db:"contact_id"`
GroupID types.Int `db:"contactgroup_id"`
ScheduleID types.Int `db:"schedule_id"`
}

icinga-notifications/internal/recipient/recipient.go

Lines 23 to 34 in 002d14d

// MarshalText implements the encoding.TextMarshaler interface to allow JSON marshaling of map[Key]T.
func (r Key) MarshalText() (text []byte, err error) {
if r.ContactID.Valid {
return []byte(fmt.Sprintf("contact_id=%d", r.ContactID.Int64)), nil
} else if r.GroupID.Valid {
return []byte(fmt.Sprintf("group_id=%d", r.GroupID.Int64)), nil
} else if r.ScheduleID.Valid {
return []byte(fmt.Sprintf("schedule_id=%d", r.ScheduleID.Int64)), nil
} else {
return nil, nil
}
}

icinga-notifications/internal/config/runtime.go

Lines 92 to 112 in 002d14d

func (r *RuntimeConfig) GetRecipient(k recipient.Key) recipient.Recipient {
// Note: be careful to return nil for non-existent IDs instead of (*T)(nil) as (*T)(nil) != nil.
if k.ContactID.Valid {
c := r.Contacts[k.ContactID.Int64]
if c != nil {
return c
}
} else if k.GroupID.Valid {
g := r.Groups[k.GroupID.Int64]
if g != nil {
return g
}
} else if k.ScheduleID.Valid {
s := r.Schedules[k.ScheduleID.Int64]
if s != nil {
return s
}
}
return nil
}

I'd want to keep that assumption in place as recipient.Key is used as a key in maps for example and that would result in nasty bugs if recipient.Key values with more than one member set would ever be used in a map key.

So I think it would be better to add the three columns as explicit members in incident.HistoryRow if it's now intended that more than one is set to a non-null value.

if !r.ContactID.Valid && recipientKey.ContactID.Valid {
r.ContactID = recipientKey.ContactID
}

if !r.GroupID.Valid && recipientKey.GroupID.Valid {
r.GroupID = recipientKey.GroupID
}

if !r.ScheduleID.Valid && recipientKey.ScheduleID.Valid {
r.ScheduleID = recipientKey.ScheduleID
}

return r
}

func ToKey(r Recipient) Key {
switch v := r.(type) {
case *Contact:
Expand Down