-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: expose extism_log_file on Plugin #34
Conversation
plugin.Call("run_test", Array.Empty<byte>()); | ||
} | ||
|
||
// HACK: tempFile gets locked by the Extism runtime |
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.
@zshipko is there any way to make the logger lock less restrictive? Currently I can't read the file even after I dispose the Plugin
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'm not sure what kind of locking is happening here but it seems kind of surprising. I'll have to read through the log4rs code, which is handling that log file.
One thing worth noting is that the logging is a global configuration so disposing the Plugin wouldn't have any effect. Maybe there is some configuration about when to flush to disk that we could update?
I also want to take a look at the logging situation this week and see if there are any paths for improvement.
bumping this, cc @zshipko -- curious how it's impacted by the |
I'm assuming logging should work better now, I haven't tested from dotnet yet. I think this PR is almost there, the only change needed is |
Since the method is static, I think it's fine. Before the big refactoring, it used to be on Context, but I can't think of a better place for it right now |
// [Fact] | ||
// Interferes with FileLog | ||
internal void CustomLog() | ||
{ | ||
var builder = new StringBuilder(); | ||
|
||
Plugin.ConfigureCustomLogging(LogLevel.Warn); | ||
using (var plugin = Helpers.LoadPlugin("log.wasm")) | ||
{ | ||
plugin.Call("run_test", Array.Empty<byte>()); | ||
} | ||
|
||
Plugin.DrainCustomLogs(line => builder.AppendLine(line)); | ||
|
||
var content = builder.ToString(); | ||
content.ShouldContain("warn"); | ||
content.ShouldContain("error"); | ||
content.ShouldNotContain("info"); | ||
content.ShouldNotContain("debug"); | ||
content.ShouldNotContain("trace"); | ||
} |
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.
@zshipko I had to comment this test out because it was interfering with the FileLog
test (they were eating each others logs). Is there any way to disable Custom/File logging?
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.
No, unfortunately it's not possible to replace the current logger once it's set up. I can look into it a bit more tomorrow but it seems like that is the default behavior of tracing-subscriber.
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.
Not sure how hard it is, but I think it would be good if they automatically unsubscribe each other
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.
We're using something like this: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/fmt/fn.try_init.html which expects to be initialized once at the beginning of the program. I will see if there's another way.
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 spent some time looking at this today and I think we could use https://docs.rs/tracing-subscriber/latest/tracing_subscriber/reload/index.html but it isn't super straight-forward. I will see if I can come up with a minimally invasive way to use that but haven't had much luck so far.
@zshipko I think we can merge this for now |
We used to expose it on Context, but Context doesn't exist anymore