From c0776e70a33ec7079e3b30a84b7324d65a7cfd31 Mon Sep 17 00:00:00 2001 From: Haw Loeung Date: Tue, 22 Nov 2022 13:57:07 +1100 Subject: [PATCH 1/2] Reduce calls to Mattermost server by caching replies --- bridge/mattermost/mattermost.go | 111 +++++++++++++++++++++++++------- mm-go-irckit/userbridge.go | 22 +------ 2 files changed, 91 insertions(+), 42 deletions(-) diff --git a/bridge/mattermost/mattermost.go b/bridge/mattermost/mattermost.go index 1c6c1442..41656136 100644 --- a/bridge/mattermost/mattermost.go +++ b/bridge/mattermost/mattermost.go @@ -6,6 +6,7 @@ import ( "fmt" "regexp" "strings" + "sync" "time" "github.com/42wim/matterircd/bridge" @@ -24,6 +25,12 @@ type Mattermost struct { eventChan chan *bridge.Event v *viper.Viper connected bool + + // Parent/root post message cache used for adding to threaded replies (unless HideReplies). + // The index is to make the size bounded so it's not unlimited. + msgMapMutex sync.RWMutex + msgMap map[string]map[int]map[string]string + msgMapIdx map[string]int } func New(v *viper.Viper, cred bridge.Credentials, eventChan chan *bridge.Event, onWsConnect func()) (bridge.Bridger, *matterclient.Client, error) { @@ -32,6 +39,8 @@ func New(v *viper.Viper, cred bridge.Credentials, eventChan chan *bridge.Event, eventChan: eventChan, v: v, } + m.msgMap = make(map[string]map[int]map[string]string) + m.msgMapIdx = make(map[string]int) logger.SetFormatter(&logger.TextFormatter{FullTimestamp: true}) if v.GetBool("debug") { @@ -756,6 +765,62 @@ func maybeShorten(msg string, newLen int, uncounted string, unicode bool) string return fmt.Sprintf("%s %s", newMsg, ellipsis) } +// XXX: Maybe make the buffer/cache size configurable? +const defaultReplyMsgCacheSize = 30 + +func (m *Mattermost) addParentMsg(parentID string, msg string, channelID string, newLen int, uncounted string, unicode bool) (string, error) { + if _, ok := m.msgMap[channelID]; !ok { + // Map doesn't exist for this channel, let's create it. + mm := make(map[int]map[string]string) + m.msgMap[channelID] = mm + m.msgMapIdx[channelID] = 0 + } + + replyMessage := "" + // Search and use cached reply if it exists. + for _, element := range m.msgMap[channelID] { + for postID, reply := range element { + if postID == parentID { + logger.Debugf("Found saved reply for parent post %s, using:%s", parentID, reply) + replyMessage = reply + } + } + } + + // None found, so we'll need to create one and save it for future uses. + if replyMessage == "" { + parentPost, _, err := m.mc.Client.GetPost(parentID, "") + // Retry once on failure. + if err != nil { + parentPost, _, err = m.mc.Client.GetPost(parentID, "") + } + if err != nil { + return msg, err + } + + parentUser := m.GetUser(parentPost.UserId) + parentMessage := maybeShorten(parentPost.Message, newLen, uncounted, unicode) + replyMessage = fmt.Sprintf(" (re @%s: %s)", parentUser.Nick, parentMessage) + + logger.Debugf("Created reply for parent post %s:%s", parentID, replyMessage) + + m.msgMapMutex.Lock() + defer m.msgMapMutex.Unlock() + + // Delete existing entry if present + delete(m.msgMap[channelID], m.msgMapIdx[channelID]) + // Now insert new + m.msgMap[channelID][m.msgMapIdx[channelID]] = make(map[string]string) + m.msgMap[channelID][m.msgMapIdx[channelID]][parentID] = replyMessage + m.msgMapIdx[channelID] += 1 + if m.msgMapIdx[channelID] > defaultReplyMsgCacheSize { + m.msgMapIdx[channelID] = 0 + } + } + + return strings.TrimRight(msg, "\n") + replyMessage, nil +} + //nolint:funlen,gocognit,gocyclo,cyclop,forcetypeassert func (m *Mattermost) handleWsActionPost(rmsg *model.WebSocketEvent) { var data model.Post @@ -771,19 +836,12 @@ func (m *Mattermost) handleWsActionPost(rmsg *model.WebSocketEvent) { return } - if data.RootId != "" { - parentPost, _, err := m.mc.Client.GetPost(data.RootId, "") + if !m.v.GetBool("mattermost.hidereplies") && data.RootId != "" { + message, err := m.addParentMsg(data.RootId, data.Message, data.ChannelId, m.v.GetInt("mattermost.ShortenRepliesTo"), "@", m.v.GetBool("mattermost.unicode")) if err != nil { logger.Errorf("Unable to get parent post for %#v", data) //nolint:govet - } else { - parentGhost := m.GetUser(parentPost.UserId) - - if !m.v.GetBool("mattermost.hidereplies") { - parentMessage := maybeShorten(parentPost.Message, m.v.GetInt("mattermost.ShortenRepliesTo"), "@", m.v.GetBool("mattermost.unicode")) - replyMessage := fmt.Sprintf(" (re @%s: %s)", parentGhost.Nick, parentMessage) - data.Message = strings.TrimRight(data.Message, "\n") + replyMessage - } } + data.Message = message } // create new "ghost" user @@ -1214,23 +1272,32 @@ func (m *Mattermost) handleReactionEvent(rmsg *model.WebSocketEvent) { return } + userID := m.GetUser(reaction.UserId) + + // No need to show added/removed reaction messages for our own. + if userID.Me { + logger.Debugf("Not showing own reaction: %s: %s", rmsg.EventType(), reaction.EmojiName) + return + } + var event *bridge.Event channelType := "" + channelID := rmsg.GetBroadcast().ChannelId - name := m.GetChannelName(rmsg.GetBroadcast().ChannelId) + name := m.GetChannelName(channelID) if strings.Contains(name, "__") { channelType = "D" } var parentUser *bridge.UserInfo - message := "" + rMessage := "" if !m.v.GetBool("mattermost.hidereplies") { - parentPost, _, err := m.mc.Client.GetPost(reaction.PostId, "") - if err == nil { - parentUser = m.GetUser(parentPost.UserId) - message = maybeShorten(parentPost.Message, m.v.GetInt("mattermost.shortenrepliesto"), "@", m.v.GetBool("mattermost.unicode")) + message, err := m.addParentMsg(reaction.PostId, "", channelID, m.v.GetInt("mattermost.ShortenRepliesTo"), "@", m.v.GetBool("mattermost.unicode")) + if err != nil { + logger.Errorf("Unable to get parent post for %#v", reaction) } + rMessage = message } switch rmsg.EventType() { @@ -1238,26 +1305,26 @@ func (m *Mattermost) handleReactionEvent(rmsg *model.WebSocketEvent) { event = &bridge.Event{ Type: "reaction_add", Data: &bridge.ReactionAddEvent{ - ChannelID: rmsg.GetBroadcast().ChannelId, + ChannelID: channelID, MessageID: reaction.PostId, - Sender: m.GetUser(reaction.UserId), + Sender: userID, Reaction: reaction.EmojiName, ChannelType: channelType, ParentUser: parentUser, - Message: message, + Message: rMessage, }, } case model.WebsocketEventReactionRemoved: event = &bridge.Event{ Type: "reaction_remove", Data: &bridge.ReactionRemoveEvent{ - ChannelID: rmsg.GetBroadcast().ChannelId, + ChannelID: channelID, MessageID: reaction.PostId, - Sender: m.GetUser(reaction.UserId), + Sender: userID, Reaction: reaction.EmojiName, ChannelType: channelType, ParentUser: parentUser, - Message: message, + Message: rMessage, }, } } diff --git a/mm-go-irckit/userbridge.go b/mm-go-irckit/userbridge.go index 2382ab1e..1079ab12 100644 --- a/mm-go-irckit/userbridge.go +++ b/mm-go-irckit/userbridge.go @@ -400,13 +400,7 @@ func (u *User) handleReactionEvent(event interface{}) { switch e := event.(type) { case *bridge.ReactionAddEvent: - if !u.v.GetBool(u.br.Protocol() + ".hidereplies") { - nick := "(none)" - if e.ParentUser != nil { - nick = sanitizeNick(e.ParentUser.Nick) - } - message = fmt.Sprintf(" (re @%s: %s)", nick, e.Message) - } + message = e.Message text = "added reaction: " channelID = e.ChannelID messageID = e.MessageID @@ -414,13 +408,7 @@ func (u *User) handleReactionEvent(event interface{}) { channelType = e.ChannelType reaction = e.Reaction case *bridge.ReactionRemoveEvent: - if !u.v.GetBool(u.br.Protocol() + ".hidereplies") { - nick := "(none)" - if e.ParentUser != nil { - nick = sanitizeNick(e.ParentUser.Nick) - } - message = fmt.Sprintf(" (re @%s: %s)", nick, e.Message) - } + message = e.Message text = "removed reaction: " channelID = e.ChannelID messageID = e.MessageID @@ -436,12 +424,6 @@ func (u *User) handleReactionEvent(event interface{}) { return } - // No need to show added/removed reaction messages for our own. - if sender.Me { - logger.Debug("Not showing own reaction: " + text + reaction) - return - } - if channelType == "D" { e := &bridge.DirectMessageEvent{ Text: text + reaction + message, From 1d7f1f5b8b60fd17d72eebc393caf7482e23c3c7 Mon Sep 17 00:00:00 2001 From: Haw Loeung Date: Sun, 4 Dec 2022 09:24:13 +1100 Subject: [PATCH 2/2] Switch to hashicorp/golang-lru removing complexity introduced per review --- bridge/mattermost/mattermost.go | 56 +++++++-------------------------- 1 file changed, 12 insertions(+), 44 deletions(-) diff --git a/bridge/mattermost/mattermost.go b/bridge/mattermost/mattermost.go index 41656136..d8654852 100644 --- a/bridge/mattermost/mattermost.go +++ b/bridge/mattermost/mattermost.go @@ -6,11 +6,11 @@ import ( "fmt" "regexp" "strings" - "sync" "time" "github.com/42wim/matterircd/bridge" "github.com/davecgh/go-spew/spew" + lru "github.com/hashicorp/golang-lru" "github.com/matterbridge/matterclient" "github.com/mattermost/mattermost-server/v6/model" "github.com/mitchellh/mapstructure" @@ -26,11 +26,7 @@ type Mattermost struct { v *viper.Viper connected bool - // Parent/root post message cache used for adding to threaded replies (unless HideReplies). - // The index is to make the size bounded so it's not unlimited. - msgMapMutex sync.RWMutex - msgMap map[string]map[int]map[string]string - msgMapIdx map[string]int + msglruCache *lru.Cache } func New(v *viper.Viper, cred bridge.Credentials, eventChan chan *bridge.Event, onWsConnect func()) (bridge.Bridger, *matterclient.Client, error) { @@ -39,8 +35,8 @@ func New(v *viper.Viper, cred bridge.Credentials, eventChan chan *bridge.Event, eventChan: eventChan, v: v, } - m.msgMap = make(map[string]map[int]map[string]string) - m.msgMapIdx = make(map[string]int) + cache, _ := lru.New(300) + m.msglruCache = cache logger.SetFormatter(&logger.TextFormatter{FullTimestamp: true}) if v.GetBool("debug") { @@ -765,30 +761,12 @@ func maybeShorten(msg string, newLen int, uncounted string, unicode bool) string return fmt.Sprintf("%s %s", newMsg, ellipsis) } -// XXX: Maybe make the buffer/cache size configurable? -const defaultReplyMsgCacheSize = 30 +func (m *Mattermost) addParentMsg(parentID string, msg string, newLen int, uncounted string, unicode bool) (string, error) { + var replyMessage string -func (m *Mattermost) addParentMsg(parentID string, msg string, channelID string, newLen int, uncounted string, unicode bool) (string, error) { - if _, ok := m.msgMap[channelID]; !ok { - // Map doesn't exist for this channel, let's create it. - mm := make(map[int]map[string]string) - m.msgMap[channelID] = mm - m.msgMapIdx[channelID] = 0 - } - - replyMessage := "" // Search and use cached reply if it exists. - for _, element := range m.msgMap[channelID] { - for postID, reply := range element { - if postID == parentID { - logger.Debugf("Found saved reply for parent post %s, using:%s", parentID, reply) - replyMessage = reply - } - } - } - // None found, so we'll need to create one and save it for future uses. - if replyMessage == "" { + if v, ok := m.msglruCache.Get(parentID); !ok { parentPost, _, err := m.mc.Client.GetPost(parentID, "") // Retry once on failure. if err != nil { @@ -801,21 +779,11 @@ func (m *Mattermost) addParentMsg(parentID string, msg string, channelID string, parentUser := m.GetUser(parentPost.UserId) parentMessage := maybeShorten(parentPost.Message, newLen, uncounted, unicode) replyMessage = fmt.Sprintf(" (re @%s: %s)", parentUser.Nick, parentMessage) - logger.Debugf("Created reply for parent post %s:%s", parentID, replyMessage) - m.msgMapMutex.Lock() - defer m.msgMapMutex.Unlock() - - // Delete existing entry if present - delete(m.msgMap[channelID], m.msgMapIdx[channelID]) - // Now insert new - m.msgMap[channelID][m.msgMapIdx[channelID]] = make(map[string]string) - m.msgMap[channelID][m.msgMapIdx[channelID]][parentID] = replyMessage - m.msgMapIdx[channelID] += 1 - if m.msgMapIdx[channelID] > defaultReplyMsgCacheSize { - m.msgMapIdx[channelID] = 0 - } + m.msglruCache.Add(parentID, replyMessage) + } else if replyMessage, ok = v.(string); ok { + logger.Debugf("Found saved reply for parent post %s, using:%s", parentID, replyMessage) } return strings.TrimRight(msg, "\n") + replyMessage, nil @@ -837,7 +805,7 @@ func (m *Mattermost) handleWsActionPost(rmsg *model.WebSocketEvent) { } if !m.v.GetBool("mattermost.hidereplies") && data.RootId != "" { - message, err := m.addParentMsg(data.RootId, data.Message, data.ChannelId, m.v.GetInt("mattermost.ShortenRepliesTo"), "@", m.v.GetBool("mattermost.unicode")) + message, err := m.addParentMsg(data.RootId, data.Message, m.v.GetInt("mattermost.ShortenRepliesTo"), "@", m.v.GetBool("mattermost.unicode")) if err != nil { logger.Errorf("Unable to get parent post for %#v", data) //nolint:govet } @@ -1293,7 +1261,7 @@ func (m *Mattermost) handleReactionEvent(rmsg *model.WebSocketEvent) { var parentUser *bridge.UserInfo rMessage := "" if !m.v.GetBool("mattermost.hidereplies") { - message, err := m.addParentMsg(reaction.PostId, "", channelID, m.v.GetInt("mattermost.ShortenRepliesTo"), "@", m.v.GetBool("mattermost.unicode")) + message, err := m.addParentMsg(reaction.PostId, "", m.v.GetInt("mattermost.ShortenRepliesTo"), "@", m.v.GetBool("mattermost.unicode")) if err != nil { logger.Errorf("Unable to get parent post for %#v", reaction) }