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

Intercept workerd logs + write all debug logs to a hidden file #4341

Merged
merged 29 commits into from
Nov 20, 2023

Conversation

RamIdeas
Copy link
Contributor

@RamIdeas RamIdeas commented Nov 3, 2023

Fixes #3631

What this PR solves / how to test:

  • fixes a bug where Miniflare's Log class rethrew the error when the log level meant the error wouldn't be logged
  • exposes a new Miniflare option: handleRuntimeStdio which allows intercepting workerd stdout/stderr
  • wrangler now intercepts workerd logs using the above option
  • wrangler now writes all debug logs to .wrangler/debug-logs/wrangler-debug-<TIMESTAMP>.log (superseding this PR)

Implementation notes:

  • Logs with log level debug are now never sent to the terminal so the terminal remains clean but the extra observability is provided via the log file
  • The default debug log filepath can be overridden using the new WRANGLER_LOG_DIR env var
    • which allows you to specify a directory path or a specific .log filepath
    • the default value for this env var is <package>/.wrangler/debug-logs
    • the filename will contain a timestamp, unless the WRANGLER_LOG_DIR value ends in .log
  • The e2e test workflow will upload the debug-log file as an artefact for inspection upon failures (primarily, but also available on successful runs too)
  • The logger level debug is what triggers the use of the log file but all logs are appended to the log file so that the order amongst other log-level logs (info, error, warn, etc) is preserved

Exposing a handleRuntimeStdio option, instead of hooks for each readline interface of the current implementation, should also satisfy #4357

Author has addressed the following:

  • Tests
    • Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • Included
    • Not necessary because:
  • Associated docs
    • Issue(s)/PR(s):
    • Not necessary because:

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

Copy link

changeset-bot bot commented Nov 3, 2023

🦋 Changeset detected

Latest commit: 664e4a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
miniflare Minor
wrangler Minor
@cloudflare/pages-shared Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Nov 3, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6932507257/npm-package-wrangler-4341

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6932507257/npm-package-wrangler-4341

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6932507257/npm-package-wrangler-4341 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6932507257/npm-package-miniflare-4341
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6932507257/npm-package-cloudflare-pages-shared-4341

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.16.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20231030.0
workerd 1.20231030.0 1.20231030.0
workerd --version 1.20231030.0 2023-10-30

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@RamIdeas RamIdeas changed the title Intercept workerd logs Intercept workerd logs + write all debug logs to a hidden file Nov 3, 2023
@RamIdeas RamIdeas added the e2e Run e2e tests on a PR label Nov 6, 2023
@RamIdeas RamIdeas force-pushed the hide-workerd-logs branch 2 times, most recently from 8e8c776 to 2cc622d Compare November 6, 2023 14:55
@RamIdeas RamIdeas marked this pull request as ready for review November 7, 2023 14:14
@RamIdeas RamIdeas requested a review from a team as a code owner November 7, 2023 14:14
@admah
Copy link
Contributor

admah commented Nov 7, 2023

@RamIdeas are users notified somewhere that this new file exists?

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Miniflare changes look good! I've added some comments to some of the Wrangler ones... 👍

packages/wrangler/src/dev/miniflare.ts Show resolved Hide resolved
packages/wrangler/src/dev/miniflare.ts Show resolved Hide resolved
packages/wrangler/src/dev/miniflare.ts Show resolved Hide resolved
packages/wrangler/src/utils/debug-log-file.ts Outdated Show resolved Hide resolved
packages/wrangler/src/logger.ts Outdated Show resolved Hide resolved
packages/wrangler/src/utils/debug-log-file.ts Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #4341 (664e4a4) into main (1747d21) will decrease coverage by 0.24%.
Report is 1 commits behind head on main.
The diff coverage is 36.48%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4341      +/-   ##
==========================================
- Coverage   75.46%   75.23%   -0.24%     
==========================================
  Files         225      226       +1     
  Lines       12482    12555      +73     
  Branches     3242     3256      +14     
==========================================
+ Hits         9420     9446      +26     
- Misses       3062     3109      +47     
Files Coverage Δ
...ages/wrangler/src/environment-variables/factory.ts 71.42% <ø> (ø)
packages/wrangler/src/logger.ts 98.27% <83.33%> (-1.73%) ⬇️
packages/wrangler/src/dev/miniflare.ts 55.20% <17.39%> (-5.15%) ⬇️
packages/wrangler/src/utils/debug-log-file.ts 40.00% <40.00%> (ø)

when the log level means it won't be logged
allowing wrangler to decide whether workerd logs should reach the terminal
packages/miniflare/src/shared/log.ts Show resolved Hide resolved
packages/wrangler/src/logger.ts Outdated Show resolved Hide resolved
packages/wrangler/src/utils/debug-log-file.ts Outdated Show resolved Hide resolved
Comment on lines +83 to +85
export function readDebugLogFile(): Promise<string> {
return mutex.runWith(() => readFile(debugLogFilepath, "utf-8"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function used anywhere?

.github/workflows/e2e.yml Outdated Show resolved Hide resolved
.github/workflows/pullrequests.yml Outdated Show resolved Hide resolved
@RamIdeas RamIdeas merged commit d990874 into main Nov 20, 2023
19 checks passed
@RamIdeas RamIdeas deleted the hide-workerd-logs branch November 20, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: "warning: Not symbolizing stack traces because $LLVM_SYMBOLIZER is not set"
5 participants