-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Logging system refactor #412
Conversation
(in case it was missed: the bot-sdk has a |
Yep, the current problem is that it lacks the ability to set freeform metadata on the Logging object which is something we're going to end up needing for the Request logging we have planned. Given the time-to-merge delays on matrix-bot-sdk, it's not something we're going to be able to depend on easily, hence running out own solution. We still wrap the bot-sdk logger so that the logs get piped nicely into one place. |
d6c2c8d
to
44d389f
Compare
d097509
to
7da88b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but the fileName
as Object key should get resolved before we merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Split out from #410
Our current logging system is hard to use, and also difficult to override. As such we've ended up building our own solutions in the Discord bridges, IRC bridge and the hookshot bridge. This PR attempts to pull in the hookshot way of doing logging into this bridge without trying to break the API entirely.
I've retained our existing log rotation transport feature, but I'm not wholly convinced it's something we want long term. Log rotation is best left to other services (KISS principle applies).
For reference https://github.com/matrix-org/matrix-hookshot/blob/main/src/LogWrapper.ts
The PR rotted enough that trying to merge some changes in broke the world, so I have restarted again on a single PR.