-
Notifications
You must be signed in to change notification settings - Fork 7
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(component): rust component model support #128
Conversation
64ebcbc
to
deb3e97
Compare
observe-sdk/rust/src/wasm_instr.rs Line 9 in 5a8d5ac
4 to match the change in wasm-instr as changing the naming is a breaking change. This change probably needs to be done on the other language sdks.
The pointers in the observe api were intentionally passed as |
I ended up aligning our API with the component model's canonical ABI, which is defined in terms of 32-bit pointers and extents. It looks like they're tracking the memory64 proposal here and here; but both issues haven't had activity in about a year. This makes the |
Thanks for the thorough explanation on the pointer passing change! That sounds reasonable. |
70d2b32
to
9def894
Compare
- upgrade to wasmtime>=13, wasmtime_wasi>=13 - the component model necessitated splitting the adapter setup into two parts: first creating the adapter so it can be associated with the state, then telling the linker how to get from the incoming state to the adapter. WASI also has this split. - change the c bindings in observe-api: 1. "_" is an illegal character! switch to "-". 2. change the module name from "dylibso_observe" to "dylibso:observe/api"
Align core module instrumentation naming with component model naming. This relies on corresponding (undeployed, private) changes to the observe http api [1]. [1]: dylibso/wasm-instr#69
This also removes the WIT `dylibso:observe` world since we don't *really* need it.
Update vendored instrumented wasm modules to match new naming. Additionally: switch github workflow from `npm i` to the (much faster) `npm ci`.
… dylibso_observe namespace
Please use test files in the `test` directory in the root of the project instead
…_observe, with warning. Error on modules instrumented with observe_api
9def894
to
e94c46f
Compare
@mhmd-azeez Could you please try out 0b43202 with the modules and adapters used on #152 ? I just finished rebasing this on top of #152 and manually reapplying my changes to the go sdk and want to be sure it's still working as intended. The only change when running modules with the log.Println("Module uses deprecated namespace \"dylibso_observe\"!\n" +
"Please consider reinstrumenting with newer wasm-instr or Observe API!") |
@G4Vi sorry, just saw your comment. It seems to be working correctly: Old modules:
New modules:
|
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.
These updates look great, @G4Vi! Thanks for taking this over the finish line.
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.
Nice, read through the changes and it looks good! Definitely seems worth merging now.
17
is the wasi preview 2 supporting releaseformat
enum doesn't match our core module format –Statsd
is 0 in the component model version.Issue: #142