From 77db4fa3e79cdb736beae63c5d6279a2686a72f9 Mon Sep 17 00:00:00 2001 From: Mateusz Szostok Date: Mon, 4 Jul 2022 13:31:20 +0200 Subject: [PATCH] Decrease the unmarshall error severity from err to warn #629 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ##### SUMMARY This PR only decreases the unmarshall error severity from err to warn. To make it a bit better, we can contribute to the `slack` lib and replace: ```go err := fmt.Errorf("RTM Error: Received unmapped event %q", typeStr) ``` _([source](https://github.com/slack-go/slack/blob/1dcd0d459a30d8402f5c0c42e20df80b56dac5e3/websocket_managed_conn.go#L477))_ with error type `UnmappedError`. This won't be a breaking change for them, so we should be able to merge that. As a result, we will know the details about the error and could implement sth like: ```go // handlingEvents holds event names which have corresponding handling logic. var handlingEvents = sets.NewString("message", "connected") // trimmed case *slack.UnmarshallingErrorEvent: err, ok := ev.ErrorObj.(slack.UnmappedError) if ok && !handlingEvents.Has(err.EventType()) { // it's not an err for us, as we don't care about this event type. b.log.Debugf("Slack unmarshalling error: %+v", ev.Error()) continue } b.log.Errorf("Slack unmarshalling error: %+v", ev.Error()) ``` In that way even if someone will configure custom Slack bot with wrong event subscription we will be able to “ignore them”. WDYT? Is it worth it? If we stay with RTM, then it's the only option. ##### FINDINGS I wanted to describe the workaround for this issue by creating the own Bot. I found that the current Bot is based on RTM. This means that it issues the [old API](https://api.slack.com/rtm). ![Screen Shot 2022-07-04 at 12 36 47](https://user-images.githubusercontent.com/17568639/177138030-04870c01-32aa-45ac-adb2-8bb05d542622.png) In this case, we are not able to: - reduce the app permission - select events to which we want to be subscribed The best option would be changing the implementation from RTM to [WebSocket mode](https://api.slack.com/apis/connections/socket). Should I create an issue? Related issue: - #619 --- pkg/bot/slack.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/bot/slack.go b/pkg/bot/slack.go index 5687cf5ab..e574ff794 100644 --- a/pkg/bot/slack.go +++ b/pkg/bot/slack.go @@ -73,7 +73,7 @@ func NewSlackBot(log logrus.FieldLogger, c *config.Config, executorFactory Execu } } -// Start starts the slacknot RTM connection and listens for messages +// Start starts the Slack RTM connection and listens for messages func (b *SlackBot) Start(ctx context.Context) error { b.log.Info("Starting bot") var botID string @@ -139,7 +139,7 @@ func (b *SlackBot) Start(ctx context.Context) error { b.log.Errorf("Slack outgoing event error: %+v", ev.Error()) case *slack.UnmarshallingErrorEvent: - b.log.Errorf("Slack unmarshalling error: %+v", ev.Error()) + b.log.Warningf("Slack unmarshalling error: %+v", ev.Error()) case *slack.RateLimitedError: b.log.Errorf("Slack rate limiting error: %+v", ev.Error())