-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
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.
Overall, looks good, thank you! I'm just unsure about why host.wasm
being regenerated. Changes may not be required with an explanation about it.
Edit: See CI failures too.
wasm/host.wasm
Outdated
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.
Why regenerate this file? host.go
doesn't look related. Was this done because the pdk was updated?
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.
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.
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 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.
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.
Indeed, I need to re-instate that first, leave it with me and will sort both. Thanks
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.
And I was the one who removed it as a temporarily fix here :(
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.
@G4Vi changes made - I wanted to see the CICD running, but I see a message saying 1 workflow awaiting approval
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.
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.
@G4Vi thank you, bear with me, I will figure out what I am missing. Thanks
github.com/extism/go-pdk v1.0.0-rc1.0.20231019212020-62d54a6e0263 // indirect | ||
github.com/valyala/fastjson v1.6.3 // indirect | ||
) | ||
require github.com/extism/go-pdk v1.0.7-0.20240804170849-7cb3d48ce7cc |
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.
pointing to this: extism/go-pdk@7cb3d48
hey just pinging this thread to see if this is blocked by anything or if I can help move it along 🙂 |
This was left on waiting for @hilaryRope to fix ci. If it isn't fixed soon we should potentially revert the pdk update. |
Hi there, sorry but I have been away. I am referencing the latest commit with my change, but I seem to be getting the same test failures. I will need to check this. Can you give me a day, please? Unless, there is something very obvious about it. Thanks |
Sure thing, thanks for the update! |
No rush at all :) didnt want this to get left behind in case it was waiting on something from our end. |
@nilslice Noo, you and @G4Vi are absolutely right and it has been on the back of my mind for a bit. I am scratching my head, perhaps stupidly, as I am pointing to the correct sha in the go-pdk where the trace has been re-instante and still I am getting a failure in Line 455 in 9748ea0
|
Thanks, @hilaryRope! I'll run CI and see what happens, and can investigate today too |
@hilaryRope @G4Vi - i'm going to merge this and make the tweaks from a new branch, as I am working on some related changes that may make this PR harder to merge later. The test looks ok, and I think it's just a problem finding the package at the ref. In any case, I won't tag a new release until all of this is sorted so these changes shouldn't impact anyone using the SDK. |
Thank you for this, appreciated! |
Description of the change
Add support for logtrace Go SDK
Issue Reference
#72 that is a logical step after this
Checklist