Slack integration for alerts and reports by esevastyanov · Pull Request #4371 · rilldata/rill

esevastyanov · 2024-03-18T18:54:45Z

alert spec has a notify section

...
notify:
  on_recover: true
  renotify: true
  renotify_after: 24h
  email:
    recipients:
      - recipient@example.com
      ...
  slack:
    emails:
      - recipient@example.com
      ...
    channels:
      - alerts
      ...
    webhooks:
      - https://...
      ...

report spec has notify section

notify:
  email:
    recipients:
      - recipient@example.com
      ...
  slack:
    emails:
      - recipient@example.com
      ...
    channels:
      - alerts
      ...
    webhooks:
      - https://...
      ...

slack driver only needs a bot token to post messages to channels and "messages" tab

start devproject --env connector.slack.bot_token=xoxb-...

Required scopes for a Slack application

Permission	Description
chat:write	Send messages
users:read	View people in a workspace (required only to search a user by an email)
users:read.email	View email addresses of people in a workspace (required only to search a user by an email)
incoming-webhook	Post messages to specific channels in Slack

esevastyanov · 2024-03-20T20:13:37Z

@AdityaHegde could you please review the change of UI components? The only thing is done there - I renamed api parameters related to email recipients of alerts and reports

k-anshul · 2024-03-21T09:42:53Z

 	}

-	// Validate recipients
+	// Validate email recipients


This gives an impression that someone can use both older format as well as newer flow but we really should allow one format.

k-anshul · 2024-03-21T10:50:08Z

+	return h, true
+}
+
+func (h *handle) SendScheduledReport(s *drivers.ScheduledReport, r drivers.RecipientOpts) error {


Would recommend moving notifier related functions to notifier.go file in this pkg or similar.

k-anshul · 2024-03-21T10:58:13Z

 		}
+
+		if a.Spec.SlackChannels != nil || a.Spec.SlackEmails != nil {
+			conn, release, err := r.C.Runtime.AcquireHandle(ctx, r.C.InstanceID, "slack")


This should not be hardcoded but should come from alert spec to support more connectors in future.

k-anshul · 2024-03-21T11:00:00Z

 			}
 		}
+
+		if a.Spec.SlackChannels != nil || a.Spec.SlackEmails != nil {


Can also have webhooks. Should also use len(a.Spec.SlackChannels) > 0 instead of nil checks.

k-anshul · 2024-03-21T11:02:55Z

  ExportFormat export_format = 8;
  repeated string email_recipients = 9;
-  map<string, string> annotations = 10;
+  repeated string slack_channels = 10;


The spec should just have a NotifierConnector and NotifierProperties instead of separate slack related fields to support different notifiers in future. Refer to parse_api.go and usage of Resolver and ResolverProperties in in APISpec for similar constructs.

k-anshul · 2024-03-21T11:06:16Z

+	}
+
+	if rep.Spec.SlackChannels != nil || rep.Spec.SlackEmails != nil {
+		conn, release, err := r.C.Runtime.AcquireHandle(ctx, r.C.InstanceID, "slack")


Same comment as in alert.go i.e. connector should be available from the spec.

k-anshul · 2024-03-21T11:09:19Z

 	}

-	if len(opts.Recipients) == 0 {
+	if len(opts.EmailRecipients) == 0 && len(opts.SlackEmails) == 0 && len(opts.SlackChannels) == 0 && len(opts.SlackWebhooks) == 0 {


Ideally there should be a deduping of SlackWebhooks too similar to emails.

I guess the ability to unsubscribe is a requirement for private notifications. As for group notifications (slack webhooks, channels), there is a way to update the corresponding subscription via EditReport

# Conflicts: # go.mod

AdityaHegde

Looks good from a UI perspective

begelundmuller · 2024-04-03T07:23:36Z

+			opts.EmailRecipients = pbutil.ToSliceString(props["recipients"].([]any))
+		case "slack":
+			opts.SlackUsers = pbutil.ToSliceString(props[slack.UsersField].([]any))
+			opts.SlackChannels = pbutil.ToSliceString(props[slack.ChannelsField].([]any))
+			opts.SlackWebhooks = pbutil.ToSliceString(props[slack.WebhooksField].([]any))


These type casts might be a little fragile. If one of them are nil, the .([]any) will panic. Probably best to have a small util function that returns nil if the cast fails (i.e. val, ok := props[key].([]any))

begelundmuller · 2024-04-03T07:24:38Z

+		case "email":
+			opts.EmailRecipients = pbutil.ToSliceString(props["recipients"].([]any))
+		case "slack":
+			opts.SlackUsers = pbutil.ToSliceString(props[slack.UsersField].([]any))
+			opts.SlackChannels = pbutil.ToSliceString(props[slack.ChannelsField].([]any))
+			opts.SlackWebhooks = pbutil.ToSliceString(props[slack.WebhooksField].([]any))


Same comment

begelundmuller · 2024-04-03T07:30:11Z

 type AlertStatus struct {
 	ToEmail        string
 	ToName         string


Maybe we should remove these fields from AlertStatus since they won't be available for channels and webhooks? (The actual email to send to will be available in the notifier props for email delivery.)

Will remove this once email notifier is created analogously to Slack notifier. Placed a todo reminder

begelundmuller · 2024-04-03T07:51:19Z

+func newNotifier(token string, props map[string]any) *notifier {
+	users := pbutil.ToSliceString(props[UsersField].([]any))
+	channels := pbutil.ToSliceString(props[ChannelsField].([]any))
+	webhooks := pbutil.ToSliceString(props[WebhooksField].([]any))


It would be cleaner to decode into a struct here using the mapstructure package (like we do for connector, source and resolver properties). It handles type conversion automatically and makes it easier to see which properties are supported by the notifier. For example:

type notifier { token string props *notifierProperties templates *template.Template } type notifierProperties { Users []string `mapstructure:"users"` Channels []string `mapstructure:"channels"` Webhooks []string `mapstructure:"webhooks"` } func newNotifier(token string, propsMap map[string]any) *notifier { props := &notifierProperties{} err := mapstructure.WeakDecode(propsMap, props) if err != nil { return nil, err } ...

Moved Slack props encode/decode into Slack notifier so that it is possible to reuse them

begelundmuller · 2024-04-03T08:10:08Z

+	for _, notifier := range rep.Spec.Notifiers {
+		switch notifier.Connector {
+		case "email":
+			recipients := pbutil.ToSliceString(notifier.Properties.AsMap()["recipients"].([]any))


Would also suggest a val, ok := ... type assertion here to keep it safe from panics

begelundmuller · 2024-04-03T08:11:35Z

+		switch notifier.Connector {
+		case "email":
+			recipients := pbutil.ToSliceString(notifier.Properties.AsMap()["recipients"].([]any))
+			for _, recipient := range recipients {


Same comment about safe assertions

begelundmuller · 2024-04-03T08:11:43Z

begelundmuller · 2024-04-03T12:15:54Z

+	// AsNotifier returns a Notifier (if the driver can serve as such) to send notifications: alerts, reports, etc.
+	// Examples: email notifier, slack notifier.
+	AsNotifier(properties *structpb.Struct) (Notifier, error)


Pass a map type here (call .AsMap() upstream) – best to deserialize protos early to use native types as much as possible

begelundmuller · 2024-04-03T12:19:12Z

+func EncodeProps(users, channels, webhooks []string) (*structpb.Struct, error) {
+	return structpb.NewStruct(map[string]any{
+		"users":    pbutil.ToSliceAny(users),
+		"channels": pbutil.ToSliceAny(channels),
+		"webhooks": pbutil.ToSliceAny(webhooks),
+	})
+}
+
+func DecodeProps(propsStruct *structpb.Struct) (*NotifierProperties, error) {


Same idea – best to use native types like map internally, and serialize/deserialze to protos only when needed (i.e. in places that read/write proto types).

begelundmuller · 2024-04-03T12:20:35Z

Also note the merge conflicts

# Conflicts: # runtime/drivers/athena/athena.go # runtime/drivers/redshift/redshift.go

begelundmuller · 2024-04-03T13:21:23Z

 	specHashTrigger := a.State.SpecHash != specHash
 	refsTrigger := a.State.RefsHash != refsHash && a.Spec.RefreshSchedule != nil && a.Spec.RefreshSchedule.RefUpdate
-	scheduleTrigger := a.State.NextRunOn != nil && !a.State.NextRunOn.AsTime().After(time.Now())
+	scheduleTrigger := a.State.NextRunOn != nil && a.State.NextRunOn.AsTime().Before(time.Now())


This is not equivalent – specifically, if NextRunOn == time.Now(), we should trigger

Reverted and fixed linter changes

This reverts commit d425870.

esevastyanov force-pushed the 4183-slack-integration branch from c305231 to 3fa8b65 Compare March 18, 2024 19:40

Slack integration for alerts

c1d3303

esevastyanov force-pushed the 4183-slack-integration branch from 3fa8b65 to c1d3303 Compare March 18, 2024 19:41

Slack integration for reports

00c3859

esevastyanov force-pushed the 4183-slack-integration branch from e1b463c to 00c3859 Compare March 18, 2024 22:00

esevastyanov marked this pull request as ready for review March 19, 2024 09:36

esevastyanov changed the title ~~Slack integration for alerts~~ Slack integration for alerts and reports Mar 19, 2024

esevastyanov added 3 commits March 20, 2024 01:21

alert yaml backward compatibility

bf206b6

report yaml backward compatibility

eee8eeb

Slack webhooks for alerts and reports

7ea68f2

esevastyanov requested review from AdityaHegde and k-anshul March 20, 2024 16:33

Docs

d2b4928

k-anshul requested changes Mar 21, 2024

View reviewed changes

begelundmuller mentioned this pull request Mar 26, 2024

Runtime: APIs to get connector details for a project #4363

Merged

esevastyanov added 8 commits March 27, 2024 02:08

Refactored alert and report specs

305ca89

Merge remote-tracking branch 'origin/main' into 4183-slack-integration

8f0f37c

# Conflicts: # go.mod

resolved a merge conflict

9b9c5fb

correspondingly updated application code

84f7d8a

prettier fix

6fca635

prettier fix

e74585d

type check

356bec1

type check

c4940e1

esevastyanov requested review from begelundmuller and k-anshul March 27, 2024 00:00

AdityaHegde approved these changes Mar 27, 2024

View reviewed changes

Removed a footer of slack templates

8573262

k-anshul reviewed Mar 28, 2024

View reviewed changes

Comment thread proto/rill/runtime/v1/resources.proto

Comment thread runtime/drivers/slack/notifier.go Outdated

Comment thread runtime/drivers/slack/notifier.go Outdated

AdityaHegde mentioned this pull request Mar 28, 2024

Add slack notification inputs to alert forms #4449

Closed

esevastyanov added 5 commits April 1, 2024 23:31

schemaless notifiers

697fe7a

type check fix

b9bd728

Merge remote-tracking branch 'origin/main' into 4183-slack-integration

6ff77fe

merge fix

6813f95

Fixed minor issues

0ba18f6

esevastyanov requested a review from begelundmuller April 1, 2024 21:15

Substituted strings with constants

cc417db

begelundmuller requested changes Apr 3, 2024

View reviewed changes

Moved Slack props encode/decode into Slack notifier

1169fb2

esevastyanov requested a review from begelundmuller April 3, 2024 11:49

Substituted a custom error with a returned one

6e9b50d

begelundmuller reviewed Apr 3, 2024

View reviewed changes

esevastyanov added 5 commits April 3, 2024 16:40

moved structbp out notifier

21133e0

Merge remote-tracking branch 'origin/main' into 4183-slack-integration

ffa9647

Merge remote-tracking branch 'origin/main' into 4183-slack-integration

9694b90

# Conflicts: # runtime/drivers/athena/athena.go # runtime/drivers/redshift/redshift.go

resolved a merge conflict

2724501

Linter fix

d425870

esevastyanov requested a review from begelundmuller April 3, 2024 12:53

begelundmuller reviewed Apr 3, 2024

View reviewed changes

esevastyanov added 2 commits April 3, 2024 17:25

Revert "Linter fix"

bdefffb

This reverts commit d425870.

Linter fix (updated)

49bbaf4

esevastyanov requested a review from begelundmuller April 3, 2024 14:00

begelundmuller approved these changes Apr 3, 2024

View reviewed changes

begelundmuller removed the request for review from k-anshul April 3, 2024 15:02

k-anshul approved these changes Apr 3, 2024

View reviewed changes

begelundmuller merged commit 17e1aa3 into main Apr 3, 2024

begelundmuller deleted the 4183-slack-integration branch April 3, 2024 15:04

begelundmuller mentioned this pull request Apr 3, 2024

Enable ListNotifierConnectors for Slack #4505

Merged

dfliess mentioned this pull request Jul 2, 2026

Add generic webhook notifier for alerts and reports #9643

Open

Uh oh!

Conversation

esevastyanov commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esevastyanov commented Mar 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdityaHegde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begelundmuller commented Apr 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

esevastyanov commented Mar 18, 2024 •

edited

Loading