Only run fixTwitterEmbeds if conditions are met
#330
Conversation
### Notes This resolves an issue where Valkyrie was posting `INFO fixTwitterEmbeds` for every message whether it matches the regex pattern or not. Now we only run the twitter embed if it matches.
discord-scripts/fix-twitter-embed.ts
Outdated
| `fixTwitterEmbeds: failed to process new message ${message.content}: ${err}`, | ||
| if (message.content?.match(twitterUrlRegExp)) { | ||
| robot.logger.info( | ||
| `fixTwitterEmbeds: processing new message ${message.content}`, |
There was a problem hiding this comment.
We should also probably never log message content unless we're debugging.
Instead, let's log the channel, from user, and timestamp.
There was a problem hiding this comment.
Also we should maybe still always log that we're processing every message, but at the debug level, and making sure that production isn't set to show debug level logs...
There was a problem hiding this comment.
Sounds good and should I also look through other helpers and move any logger.info over to logger.debug when we want to separate that? On a case-by-case basis ofc!
There was a problem hiding this comment.
Yes, def makes sense to adjust log level.
What we need to peek at is what changes we might need to make so that debug shows on local testing instances but not on production instances.
There was a problem hiding this comment.
I ended up adding robot.logger.debug to the mix so that we only log those if this flag is set export HUBOT_LOG_LEVEL="debug" which is nicely already built into Hubot! This is probably a good time to start moving other scripts over to the debug level as well. 🥲
There was a problem hiding this comment.
We still need to not log message content on info level though. I think we want to:
- Change this log to only log channel,
from, and timestamp. - Maybe, add a debug log that logs the message content.
discord-scripts/fix-twitter-embed.ts
Outdated
| oldMessage?.content?.match(twitterUrlRegExp) | ||
| ) { | ||
| robot.logger.info( | ||
| `fixTwitterEmbeds: processing updated message ${newMessage.content}`, |
There was a problem hiding this comment.
Looks like we now log this twice? And same note as above re: leaving message content out of the info level.
discord-scripts/fix-twitter-embed.ts
Outdated
| workingTwitterEmbeds(newMessage, robot.logger, oldMessage).catch( | ||
| (err) => { | ||
| robot.logger.error( | ||
| `fixTwitterEmbeds: failed to process updated message ${newMessage.content}: ${err}`, |
There was a problem hiding this comment.
Same deal re: message content.
Actually I would also log the message id at info level, and only log the message content once alongside message id. That way we don't need to repeatedly spit the message content into the console.
There was a problem hiding this comment.
Just pushed a new commit with a bunch of refactoring as well hiding the whole content behind the logger.debug and then for logger.info we take user, channel, messageId and timestamp!
This changes the `fixTwitterEmbeds` to run a main `processTwitterMessage` where we output the specific user, channel, timestamp and message id to `logger.info` while outputting the entire message to `logger.debug`
| ) | ||
| } | ||
| } | ||
|
|
||
| discordClient.on("messageCreate", (message) => { | ||
| robot.logger.debug( | ||
| `fixTwitterEmbeds: processing new message ${message.content}`, |
There was a problem hiding this comment.
Let's include the message id here so we can cross-correlate to the other logs.
Notes
This resolves an issue where Valkyrie was posting
INFO fixTwitterEmbedsfor every message whether it matches the regex pattern or not. Now we only run the twitter embed if it matches.