feat: replace log with tracing#1460
Conversation
chore: merge with upstream
|
Since the global tracing-subscriber doesn't spawn to any threads spawned (post for context), each plugin spawns their own tracing_subscriber and calls a log-function from the plugin context instead of log! directly. The output will look like this: 2026-02-08 17:51:49 INFO ThreadId(13) pumpkin_plugin: Hello! plugin="hello-pumpkin"with "hello-plugin" being the name of the plugin |
BjornTheProgrammer
left a comment
BjornTheProgrammer
left a comment
There was a problem hiding this comment.
A lot of great work here! Just a few questions before this can be merged.
| match self { | ||
| Self::Console => log::info!("{}", text.to_pretty_console()), | ||
| #[allow(clippy::print_stdout)] | ||
| Self::Console => println!("{}", text.to_pretty_console()), |
There was a problem hiding this comment.
What is the reason as to using a println instead of a tracing::info?
There was a problem hiding this comment.
Ahh that was before #1516 got merged (downgrading tracing-subscriber to a version which didn't escape ANSI-characters), but I'm not sure if I want to revert this
The argument for escaping ANSI-characters by the project was for security and for commands run by users in the console, they prob don't need the extra info from tracing
| } | ||
| } | ||
|
|
||
| pub fn log(&self, message: impl std::fmt::Display) { |
There was a problem hiding this comment.
Do we want to not use a macro instead? It's a little annoying to have to do server.log(format!(...)). Additionally maybe we shouldn't even have this since logging directly in the plugin using tracing or log seems to work just fine.
There was a problem hiding this comment.
True that will be tedious, I'll try and create a macro a bit later!
Regarding the last point, I think we should consolidate as many methods a plugin might use within Pumpkin itself, standardizing logs and whatever other functionality in the future (say log rate limiting or an api for getting the logs)
There was a problem hiding this comment.
Okay, I understand the rational, but I'll wait for Alex's input on this. It might be a week before we can get this merged!
There was a problem hiding this comment.
We've decided that this is actually fine.
|
Forgot to mention it here, but I have run it on a simple plugin, and it seems to work perfectly in my testing. Great work! |
|
I wonder too if we should consider adding a step in the CI to compile the Hello-Pumpkin plugin as a way to detect breakages with plugins (like with how it's broken now because CommandExecute now expects an integer on return) Not in this pr, but a new one if useful? |
chore: merge upstream
|
I've actually tried to do the same thing in the past, but it wasn't merged. Alex wants to keep all plugins in the separate repo. Even if it is for testing, if I remember correctly. |
chore: merge upstream
Oh that can work still like in #1530, we can just point to the repository using the checkout action EDIT: it'll fail now because the plugin is currently broken |
chore: merge with upstream
Purdze
left a comment
Purdze
left a comment
There was a problem hiding this comment.
if we're using tracking we should use something like (info!(player = %name, "conneted"))
that can be done in a seperate pr though, leave a todo mentioning this and ill merge
That's a good idea, there are prob other places as well where structured logging would make sense! I'll look through later, for now the todo is added |
3 participants
Description
Replace all instances of log with tracing
Testing
WIP