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

Implement CustomLogPipes for Spin Logging/IO #471

Closed
wants to merge 14 commits into from

Conversation

danbugs
Copy link
Contributor

@danbugs danbugs commented May 11, 2022

That's exactly right. If you have your WasiFiles already, you can call ModuleIoRedirects::new directly, and pass that to prepare_component. (At least, you'll be able to if that PR is accepted.)

WAGI does this in a very simple form because it needs different behaviour for stdin, and a slightly different interaction with the buffer for stdout. It's really valuable to know that this is more broadly useful as a public customisation point though!

To provide an update on this from #438 (and maybe close that issue soon), it turns out that I couldn't really make the current architecture use our own pipes — As previously implied, the meat and potatoes of logging is done in execute:
https://github.com/fermyon/spin/blob/c3ff7d90fe115daa3fa85ce3e626c738e7baef76/crates/http/src/spin.rs#L19-L59

It's there that capture_io_to_memory, prepare_component (that receives redirects), and, subsequently, redirect_to_mem_buffer are called.

Considering our builder is set w/ with_engine(), as per:
https://github.com/fermyon/spin/blob/1fbfb23b4e96a580d6d4f4b6d617e916974493f1/crates/engine/src/lib.rs#L103-L119

I tried emulating the work done in execute in the build stage similar to how the store does it:
https://github.com/fermyon/spin/blob/0f51ad0b86b798ae18cb4f211f0c3fcfbbe9a156/crates/engine/src/lib.rs#L304-L316

... but, nothing came of it as execute would eventually run and override/never access my RuntimeContext WASI context data.

All and all, this PR allows users to set CustomLogPipes when creating their ExecutionContextConfiguration — these CustomLogPipes are later accessed by execute and passed in to capture_io_to_memory (which has a side-effect on prepare_component) providing a good and needed override of Spin's default settings.

@Mossaka
Copy link
Contributor

Mossaka commented May 11, 2022

@lann @itowlson @radu-matei for review~

@danbugs danbugs force-pushed the danbugs/custom-log-pipes branch from e5864db to 578a904 Compare May 12, 2022 16:35
@danbugs
Copy link
Contributor Author

danbugs commented May 12, 2022

Note: I had to do a force push to fix commits that weren't signed-off~ I was under the impression that having a git config --global set w/ user.email and user.name made it so that -s in git commit wasn't necessary 🤷*

Edit: Initially I did a lazy git rebase HEAD~10 --sign-off and that signed-off some commits that weren't mine, fixed that later!

@danbugs danbugs force-pushed the danbugs/custom-log-pipes branch from 578a904 to 675b211 Compare May 12, 2022 16:46
lann and others added 8 commits May 12, 2022 09:49
…ns (spinframework#422)

* added wildcard host and a rust outbound http example
* updated outbound http upstream

Signed-off-by: Jiaxiao Zhou <[email protected]>
- Adds integration test for Go SDK.
- Builds TinyGo examples

CI will only run these tests on ubuntu because it's platform agnostic.

Signed-off-by: Adam Reese <[email protected]>
Signed-off-by: danbugs <[email protected]>
Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/custom-log-pipes branch from 675b211 to 0e39baf Compare May 12, 2022 16:49
@lann
Copy link
Collaborator

lann commented May 12, 2022

@danbugs Thanks for the PR! We're currently working on the 0.2 release so might not get to this until tomorrow or Monday.

@danbugs
Copy link
Contributor Author

danbugs commented May 12, 2022

⚠️I think my rebases made this PR a bit messy, so I remade it here at #482 for ease of review~

cc: @Mossaka , @itowlson , and @lann

@danbugs danbugs deleted the danbugs/custom-log-pipes branch May 12, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants