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

Explore the new handleRuntimeStdio to ingest workerd logs #2574

Closed
wants to merge 10 commits into from

Conversation

frandiox
Copy link
Contributor

Related: cloudflare/workers-sdk#4357

Context: when we implemented the MiniOxygen version that wraps workerd, the latter didn't support log streaming. We had to add some custom code to get logs from workerd's V8 Inspector (like a debugger behind the scenes) via WebSocket events (e.g. we get a "console.log called" event, then we manually call console.log in Node). This is similar to what Wrangler was doing at the time.

Since that initial implementation, workerd has added log streaming and Miniflare now provides a handleRuntimeStdio option where we can ingest logs. This PR tried to simplify our story around logs to use this new utility, and it's relatively similar in feature parity at this point with main branch.

However, after testing it in this PR, I've found that it doesn't reduce much of the complexity. Therefore, I'm leaving this PR here for the future reference but won't merge it. Some of the issues with the new log ingestion strategy:

  1. It does not show unhandled exceptions. We'd need to run in verbose mode to get these directly from workerd, and that means we also need to add a lot of filtering to hide other cryptic verbose logs.
  2. The log streams are hard to parse because workerd waits for a buffer to be filled before sending the logs. This means multiple calls to console.log might be ingested as a single log and is hard to split later. We currently rely on structured logs so that we can enhance them in the CLI (e.g. show banners, dedup certain warnings, enhance GraphQL errors with links to GraphiQL, etc.):
  3. console.warn and console.error are mixed into stderr and is not possible to tell them apart (we rely on this for similar reasons to 2).
  4. And a few other things that made the switch more complex that the previous code.

One of the things I tried to do to workaround 2 and 3 is adding an extra parameter to every console.log call so it's later possible to know the log order and the console method used. It works in this PR, but adds more complexity because we need to inject code in-worker to wrap console methods.

@frandiox frandiox closed this Sep 30, 2024
Copy link
Contributor

shopify bot commented Sep 30, 2024

Oxygen deployed a preview of your fd-workerd-update-logger branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
custom-cart-method ✅ Successful (Logs) Preview deployment Inspect deployment September 30, 2024 9:25 AM
classic-remix ✅ Successful (Logs) Preview deployment Inspect deployment September 30, 2024 9:25 AM
sitemap ✅ Successful (Logs) Preview deployment Inspect deployment September 30, 2024 9:25 AM
Skeleton (skeleton.hydrogen.shop) ✅ Successful (Logs) Preview deployment Inspect deployment September 30, 2024 9:25 AM
metaobjects ✅ Successful (Logs) Preview deployment Inspect deployment September 30, 2024 9:25 AM
third-party-queries-caching ✅ Successful (Logs) Preview deployment Inspect deployment September 30, 2024 9:25 AM

Learn more about Hydrogen's GitHub integration.

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.

1 participant