Skip to content
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: support trace log level #73

Merged
merged 8 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions extism_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ func TestLog_default(t *testing.T) {
logs := buf.String()

assert.Contains(t, logs, "this is a warning log")
assert.Contains(t, logs, "this is an erorr log")
assert.Contains(t, logs, "this is an error log")
}
}
}
Expand All @@ -474,6 +474,8 @@ func TestLog_custom(t *testing.T) {
assert.Equal(t, fmt.Sprintf("%s", level), "WARN")
case LogLevelError:
assert.Equal(t, fmt.Sprintf("%s", level), "ERROR")
case LogLevelTrace:
assert.Equal(t, fmt.Sprintf("%s", level), "TRACE")
}
})

Expand All @@ -485,7 +487,8 @@ func TestLog_custom(t *testing.T) {
expected := []LogEntry{
{message: "this is an info log", level: LogLevelInfo},
{message: "this is a warning log", level: LogLevelWarn},
{message: "this is an erorr log", level: LogLevelError}}
{message: "this is an error log", level: LogLevelError},
{message: "this is an trace log", level: LogLevelTrace}}

assert.Equal(t, expected, actual)
}
Expand Down
3 changes: 2 additions & 1 deletion host.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ func buildEnvModule(ctx context.Context, rt wazero.Runtime, extism api.Module) (
if plugin, ok := ctx.Value("plugin").(*Plugin); ok {
message, err := plugin.currentPlugin().ReadString(offset)
if err != nil {
panic(fmt.Errorf("Failed to read log message from memory: %v", err))
panic(fmt.Errorf("failed to read log message from memory: %v", err))
}

plugin.Log(level, message)
Expand All @@ -325,6 +325,7 @@ func buildEnvModule(ctx context.Context, rt wazero.Runtime, extism api.Module) (
logFunc("log_info", LogLevelInfo)
logFunc("log_warn", LogLevelWarn)
logFunc("log_error", LogLevelError)
logFunc("log_trace", LogLevelTrace)

return builder.Instantiate(ctx)
}
Expand Down
1 change: 1 addition & 0 deletions plugins/log/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ func run_test() int32 {
pdk.Log(pdk.LogInfo, "this is an info log")
pdk.Log(pdk.LogWarn, "this is a warning log")
pdk.Log(pdk.LogError, "this is an error log")
pdk.Log(pdk.LogTrace, "this is a trace log")

return 0
}
Expand Down
Binary file modified wasm/host.wasm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why regenerate this file? host.go doesn't look related. Was this done because the pdk was updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, no, I agree, this PR was still WIP, but made a mistake and did not set it to Draft. It is not needed here. I need to check the test failures as I am not quite sure why is that. I used Modsurfer to look into the generate log.wasm and I don't see the log_trace and I believe that is why the test is failing. Not quite sure why, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, the lack of log_trace does appear to be at least part of the problem, it appears changes need to be made to the Go PDK to make log_trace available there. For example, https://github.com/extism/go-pdk/blob/main/env.go, does not import it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I need to re-instate that first, leave it with me and will sort both. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I was the one who removed it as a temporarily fix here :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@G4Vi changes made - I wanted to see the CICD running, but I see a message saying 1 workflow awaiting approval

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@G4Vi thank you, bear with me, I will figure out what I am missing. Thanks

Binary file not shown.
Binary file modified wasm/log.wasm
Binary file not shown.
Loading