-
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
Changes from all commits
6776b28
4dd761a
df926ae
b7cd8d5
a990d59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
using Extism.Sdk.Native; | ||
|
||
using Shouldly; | ||
|
||
using System.Runtime.InteropServices; | ||
using System.Text; | ||
|
||
|
@@ -178,6 +180,51 @@ public void HostFunctionsWithMemory() | |
Encoding.UTF8.GetString(response).ShouldBe("HELLO FRODO!"); | ||
} | ||
|
||
[Fact] | ||
public void FileLog() | ||
{ | ||
var tempFile = Path.GetTempFileName(); | ||
Plugin.ConfigureFileLogging(tempFile, LogLevel.Warn); | ||
using (var plugin = Helpers.LoadPlugin("log.wasm")) | ||
{ | ||
plugin.Call("run_test", Array.Empty<byte>()); | ||
} | ||
|
||
// HACK: tempFile gets locked by the Extism runtime | ||
var tempFile2 = Path.GetTempFileName(); | ||
File.Copy(tempFile, tempFile2, true); | ||
|
||
var content = File.ReadAllText(tempFile2); | ||
content.ShouldContain("warn"); | ||
content.ShouldContain("error"); | ||
content.ShouldNotContain("info"); | ||
content.ShouldNotContain("debug"); | ||
content.ShouldNotContain("trace"); | ||
} | ||
|
||
|
||
// [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"); | ||
} | ||
Comment on lines
+206
to
+226
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
|
||
public class CountVowelsResponse | ||
{ | ||
public int Count { get; set; } | ||
|
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.