Log worker script startup time #6318

danlapid · 2024-07-23T14:28:41Z

What this PR solves / how to test

Adds a log for worker startup time as per EW-8590.

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: we just add a log line to give users more information

changeset-bot · 2024-07-23T14:28:44Z

🦋 Changeset detected

Latest commit: 6f2c888

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

This PR includes changesets to release 2 packages

Name	Type
wrangler	Minor
@cloudflare/vitest-pool-workers	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

github-actions · 2024-07-23T14:31:04Z

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/10114633771/npm-package-wrangler-6318

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10114633771/npm-package-wrangler-6318 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10114633771/npm-package-create-cloudflare-6318 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10114633771/npm-package-cloudflare-kv-asset-handler-6318

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10114633771/npm-package-miniflare-6318

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10114633771/npm-package-cloudflare-pages-shared-6318

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10114633771/npm-package-cloudflare-vitest-pool-workers-6318

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

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240718.1
`workerd`	1.20240718.0	1.20240718.0
`workerd --version`	1.20240718.0	2024-07-18

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

kflansburg · 2024-07-23T15:03:06Z

@taylorlee is this number reliable?

irvinebroque · 2024-07-23T16:23:57Z

packages/wrangler/src/deploy/deploy.ts

@@ -737,6 738,8 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 })
 );

+ logger.log("Script Startup Time: ", result.startup_time_ms, "ms");


We should say "Worker" rather than "Script"

Switched it

irvinebroque · 2024-07-23T16:26:11Z

Would like to preview before we merge and release

tanushree-sharma · 2024-07-23T19:27:38Z

I'd prefer the startup time to be logged ahead of the bindings that we report on upload. Fits naturally to be the next metric after Worker size too!

tanushree-sharma · 2024-07-23T20:50:54Z

Thanks @danlapid

Before:

After:

irvinebroque

LGTM will let someone on Authoring team ✅

petebacondarwin · 2024-07-26T11:49:12Z

packages/wrangler/src/deploy/deploy.ts

@@ -737,6 738,9 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 })
 );

+ logger.log("Worker Startup Time: ", result.startup_time_ms, "ms");


In all the tests this value is undefined. That doesn't seem right?
Are those tests providing a mock response for the deployment?
Does that mock need to be updated with the startup_time_ms?

petebacondarwin · 2024-07-26T11:50:09Z

packages/wrangler/src/deploy/deploy.ts

@@ -760,6 764,7 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 }
 }
 } catch (err) {
+ printBindings({ ...withoutStaticAssets, vars: maskedVars });


Having this in the catch... could it result in printing the bindings twice? What happens if an error is thrown after the bindings have already been printed?

It seemed to me like the code after printBindings isn't fallible as it's just a few prints, I guess that could change in the future.
I could add a flag to check if the bindings have already been printed if you feel like it's more appropriate.

Added a flag.

penalosa · 2024-07-26T11:50:17Z

@danlapid I've re-added the PR template—would you be able to fill it out? In particular, this change will probably need a changeset

Same as cloudflare#6318 , but for version upload Do not merge until API release is out (probably Jul 31).

Same as #6318 , but for version upload

* [wrangler] log startup time in version upload Same as #6318 , but for version upload * [wrangler] remove bundle size warning Bundle size was a proxy for startup time. Now that we have startup time reported, focus on bundle size is less relevant. Tracked internally as EW-8219

danlapid requested a review from a team as a code owner July 23, 2024 14:28

irvinebroque reviewed Jul 23, 2024

View reviewed changes

irvinebroque requested a review from petebacondarwin July 23, 2024 16:25

irvinebroque reviewed Jul 24, 2024

View reviewed changes

petebacondarwin marked this pull request as draft July 25, 2024 15:01

danlapid force-pushed the danlapid/log_worker_startup_time branch 3 times, most recently from 9bd6058 to 638cded Compare July 26, 2024 11:44

petebacondarwin reviewed Jul 26, 2024

View reviewed changes

danlapid force-pushed the danlapid/log_worker_startup_time branch from 638cded to ad0439f Compare July 26, 2024 12:39

danlapid marked this pull request as ready for review July 26, 2024 12:48

danlapid force-pushed the danlapid/log_worker_startup_time branch from ad0439f to 9006d88 Compare July 26, 2024 12:57

Log worker script startup time

6f2c888

danlapid force-pushed the danlapid/log_worker_startup_time branch from 9006d88 to 6f2c888 Compare July 26, 2024 16:38

petebacondarwin approved these changes Jul 29, 2024

View reviewed changes

petebacondarwin merged commit dc576c8 into cloudflare:main Jul 29, 2024
18 checks passed

workers-devprod mentioned this pull request Jul 29, 2024

Version Packages #6358

Merged

taylorlee added a commit to taylorlee/workers-sdk that referenced this pull request Jul 30, 2024

[wrangler] log startup time in version upload

c76a31c

Same as cloudflare#6318 , but for version upload Do not merge until API release is out (probably Jul 31).

taylorlee mentioned this pull request Jul 30, 2024

print startup time in version upload #6381

Closed

13 tasks

taylorlee added a commit that referenced this pull request Jul 31, 2024

[wrangler] log startup time in version upload

7c45ea2

Same as #6318 , but for version upload

taylorlee mentioned this pull request Jul 31, 2024

print startup time in version upload #6392

Merged

13 tasks

taylorlee added a commit that referenced this pull request Jul 31, 2024

[wrangler] log startup time in version upload

8722ae3

Same as #6318 , but for version upload

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log worker script startup time #6318

Log worker script startup time #6318

danlapid commented Jul 23, 2024 •

edited

Loading

changeset-bot bot commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

kflansburg commented Jul 23, 2024

irvinebroque Jul 23, 2024

danlapid Jul 23, 2024

irvinebroque commented Jul 23, 2024

tanushree-sharma commented Jul 23, 2024

tanushree-sharma commented Jul 23, 2024

irvinebroque left a comment

petebacondarwin Jul 26, 2024

danlapid Jul 26, 2024

petebacondarwin Jul 26, 2024

danlapid Jul 26, 2024

danlapid Jul 26, 2024

penalosa commented Jul 26, 2024

Log worker script startup time #6318

Log worker script startup time #6318

Conversation

danlapid commented Jul 23, 2024 • edited Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Jul 23, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jul 23, 2024 • edited Loading

kflansburg commented Jul 23, 2024

irvinebroque Jul 23, 2024

Choose a reason for hiding this comment

danlapid Jul 23, 2024

Choose a reason for hiding this comment

irvinebroque commented Jul 23, 2024

tanushree-sharma commented Jul 23, 2024

tanushree-sharma commented Jul 23, 2024

irvinebroque left a comment

Choose a reason for hiding this comment

petebacondarwin Jul 26, 2024

Choose a reason for hiding this comment

danlapid Jul 26, 2024

Choose a reason for hiding this comment

petebacondarwin Jul 26, 2024

Choose a reason for hiding this comment

danlapid Jul 26, 2024

Choose a reason for hiding this comment

danlapid Jul 26, 2024

Choose a reason for hiding this comment

penalosa commented Jul 26, 2024

danlapid commented Jul 23, 2024 •

edited

Loading

changeset-bot bot commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading