-
-
Notifications
You must be signed in to change notification settings - Fork 111
fix: use logging macros instead of emitting event directly, so that it is also logged by tracing #7459
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
base: main
Are you sure you want to change the base?
Conversation
is also logged by tracing The events are needed when you are not using chatmail core from rust, if you use chatmail core from your rust bot or from tauri, then you likely already use the rust logging/tracing ecosystem. So it makes sense to use it instead of listening to the events and logging them yourself. This pr fixes a few cases where the event was direclty emitted instead of using the macro and thus was not also automatically logged via tracing.
the macro, because they can't capture self.
| "Starting background fetch for {n_accounts} accounts." | ||
| )), | ||
| }); | ||
| ::tracing::event!( |
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.
Can we add some function to Events that also does this, to avoid code duplication?
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.
normally it is inside of the logging macros, but the problem here is that we would need to either adapt the macro or duplicate and modify it to allow calling it without a structure/type with a .get_id(&self) method.
I don't felt confident doing either of those bigger changes/decisions.
The methods here are special, because they don't want to capture self.
Another idea could be a dummy struct that would provide a .get_id(&self) which just returns 0, but that feels like a hack to me.
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.
I think we need a trait for ctx passed into logging macros. Currently Context is actually an account and also acts as a logging context, we should split these concepts. This is not a subject for this PR of course. Another benefit of such a logging context may be storing some logging prefix or activity id which is logged as a log line prefix. This way we can make logs structured which may be useful for multi-transport, otherwise it may not be clear in the context of which transport an event occurs.
The events are needed when you are not using chatmail core from rust, if
you use chatmail core from your rust bot or from tauri, then you likely
already use the rust logging/tracing ecosystem. So it makes sense to use
it instead of listening to the events and logging them yourself.
This pr fixes a few cases where the event was direclty emitted instead
of using the macro and thus was not also automatically logged via
tracing.