Conversation
🦋 Changeset detectedLatest commit: 3b31d42 The changes in this PR will be included in the next version bump. 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 |
Changeset ReviewReviewed Changeset:
|
| Check | Status | Notes |
|---|---|---|
| Version Type | ✅ | Correctly marked as minor - telemetry/analytics changes should be minor per guidelines |
| Changelog Quality | ✅ | Meaningful description with clear explanation of what's collected, how to disable it, and event schema |
| Markdown Headers | ✅ | No h1/h2/h3 headers found; uses bold text for emphasis which is correct |
| Analytics | ✅ | Minor bump is appropriate for telemetry collection changes |
| Dependabot | N/A | Not a dependency update changeset |
| Experimental Features | ✅ | Explicitly notes that Local Explorer and local REST API are experimental |
Notes
- Good inclusion of the event schema for transparency
- Clear instructions on how to disable telemetry (
wrangler telemetry disable) - Appropriately scoped to only when dev session is started via Wrangler
|
I'm Bonk, and I've done a quick review of your PR. This PR adds anonymous telemetry to the Local Explorer, sending usage events to Sparrow for successful API requests. The implementation is clean — route name mapping, fire-and-forget event dispatch, and piggybacking off wrangler's existing I posted one actionable issue:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
277b092 to
9810c0f
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers |
petebacondarwin
left a comment
There was a problem hiding this comment.
Looking good - a few things that are maybe not quite blocking but I thought I would be defensive and not officially approve yet.
packages/miniflare/test/plugins/local-explorer/telemetry.spec.ts
Outdated
Show resolved
Hide resolved
ac13605 to
14c334c
Compare
petebacondarwin
left a comment
There was a problem hiding this comment.
Sweet and simple. Need to update that changeset though
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
I would recommend reviewing the first commit separately to the rest - there I’ve extracted telemetry/metrics helpers from wrangler into workers-utils.edit: un-moved this to workers-utils.We then pass these options into miniflare via a new ‘telemetry’ option. We can’t just directly get the telemetry settings in miniflare, because users can set telemetry config in their wrangler config.
Telemetry settings are then passed to the explorer worker via a JSON binding.
We collect telemetry in the API, rather than in the UI, because we want to detect API-only usage as well.
This is done by adding a middleware component that sends an event after all API requests. The telemetry event schema can be seen in the changes description.
Telemetry is only collected when accessed via wrangler dev, not via the vite plugin or standalone miniflare.
A picture of a cute animal (not mandatory, but encouraged)