Skip to content

Commit

Permalink
Decrease the unmarshall error severity from err to warn #629
Browse files Browse the repository at this point in the history
##### 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
  • Loading branch information
mszostok committed Jul 4, 2022
1 parent e683937 commit 77db4fa
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions pkg/bot/slack.go
Expand Up @@ -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
Expand Down Expand Up @@ -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())
Expand Down

0 comments on commit 77db4fa

Please sign in to comment.