From 949b7b08290754c918d2d474982aa5c41634a672 Mon Sep 17 00:00:00 2001 From: Will Taylor Date: Fri, 24 Jan 2025 16:19:44 -0500 Subject: [PATCH] Fix: Router-worker loss of sentry + metrics on error --- .changeset/olive-tigers-raise.md | 5 ++ .../asset-worker/src/handler.ts | 2 +- .../workers-shared/asset-worker/src/index.ts | 8 +-- .../router-worker/src/analytics.ts | 9 ++- .../workers-shared/router-worker/src/index.ts | 72 +++++++++++-------- .../{asset-worker/src => utils}/responses.ts | 0 6 files changed, 58 insertions(+), 38 deletions(-) create mode 100644 .changeset/olive-tigers-raise.md rename packages/workers-shared/{asset-worker/src => utils}/responses.ts (100%) diff --git a/.changeset/olive-tigers-raise.md b/.changeset/olive-tigers-raise.md new file mode 100644 index 000000000000..9abadfdb126f --- /dev/null +++ b/.changeset/olive-tigers-raise.md @@ -0,0 +1,5 @@ +--- +"@cloudflare/workers-shared": minor +--- + +Fixes bug in router-worker that prevents unexpected errors from being captured. diff --git a/packages/workers-shared/asset-worker/src/handler.ts b/packages/workers-shared/asset-worker/src/handler.ts index d43724dab4f4..49f4e7bd614e 100644 --- a/packages/workers-shared/asset-worker/src/handler.ts +++ b/packages/workers-shared/asset-worker/src/handler.ts @@ -5,7 +5,7 @@ import { NotModifiedResponse, OkResponse, TemporaryRedirectResponse, -} from "./responses"; +} from "../../utils/responses"; import { getHeaders } from "./utils/headers"; import type { AssetConfig } from "../../utils/types"; import type EntrypointType from "./index"; diff --git a/packages/workers-shared/asset-worker/src/index.ts b/packages/workers-shared/asset-worker/src/index.ts index 953617022e32..3117d9ce6eaa 100644 --- a/packages/workers-shared/asset-worker/src/index.ts +++ b/packages/workers-shared/asset-worker/src/index.ts @@ -1,12 +1,12 @@ import { WorkerEntrypoint } from "cloudflare:workers"; import { PerformanceTimer } from "../../utils/performance"; +import { InternalServerErrorResponse } from "../../utils/responses"; import { setupSentry } from "../../utils/sentry"; import { mockJaegerBinding } from "../../utils/tracing"; import { Analytics } from "./analytics"; import { AssetsManifest } from "./assets-manifest"; import { applyConfigurationDefaults } from "./configuration"; import { decodePath, getIntent, handleRequest } from "./handler"; -import { InternalServerErrorResponse } from "./responses"; import { getAssetWithMetadataFromKV } from "./utils/kv"; import type { AssetWorkerConfig, @@ -106,7 +106,7 @@ export default class extends WorkerEntrypoint { }); } - return this.env.JAEGER.enterSpan("handleRequest", async (span) => { + return await this.env.JAEGER.enterSpan("handleRequest", async (span) => { span.setTags({ hostname: url.hostname, eyeballPath: url.pathname, @@ -121,9 +121,7 @@ export default class extends WorkerEntrypoint { this.unstable_exists.bind(this), this.unstable_getByETag.bind(this) ); - }) - .catch((err) => this.handleError(sentry, analytics, err)) - .finally(() => this.submitMetrics(analytics, performance, startTimeMs)); + }); } catch (err) { const errorResponse = this.handleError(sentry, analytics, err); this.submitMetrics(analytics, performance, startTimeMs); diff --git a/packages/workers-shared/router-worker/src/analytics.ts b/packages/workers-shared/router-worker/src/analytics.ts index a23d5ba35a0d..e8c4af862f5e 100644 --- a/packages/workers-shared/router-worker/src/analytics.ts +++ b/packages/workers-shared/router-worker/src/analytics.ts @@ -42,6 +42,7 @@ type Data = { export class Analytics { private data: Data = {}; private readyAnalytics?: ReadyAnalytics; + private hasWritten: boolean = false; constructor(readyAnalytics?: ReadyAnalytics) { this.readyAnalytics = readyAnalytics; @@ -56,10 +57,16 @@ export class Analytics { } write() { - if (!this.readyAnalytics) { + if (this.hasWritten) { + // We've already written analytics, don't double send + return; + } else if (!this.readyAnalytics) { + // Local environment, no-op return; } + this.hasWritten = true; + this.readyAnalytics.logEvent({ version: VERSION, accountId: this.data.accountId, diff --git a/packages/workers-shared/router-worker/src/index.ts b/packages/workers-shared/router-worker/src/index.ts index 8235d5b404e3..fa9f21a16727 100644 --- a/packages/workers-shared/router-worker/src/index.ts +++ b/packages/workers-shared/router-worker/src/index.ts @@ -30,6 +30,7 @@ interface Env { export default { async fetch(request: Request, env: Env, ctx: ExecutionContext) { let sentry: ReturnType | undefined; + let userWorkerInvocation = false; const analytics = new Analytics(env.ANALYTICS); const performance = new PerformanceTimer(env.UNSAFE_PERFORMANCE); const startTimeMs = performance.now(); @@ -73,50 +74,59 @@ export default { // User's configuration indicates they want user-Worker to run ahead of any // assets. Do not provide any fallback logic. if (env.CONFIG.invoke_user_worker_ahead_of_assets) { - return env.JAEGER.enterSpan("invoke_user_worker_first", async () => { - if (!env.CONFIG.has_user_worker) { - throw new Error( - "Fetch for user worker without having a user worker binding" - ); - } + if (!env.CONFIG.has_user_worker) { + throw new Error( + "Fetch for user worker without having a user worker binding" + ); + } + + analytics.setData({ dispatchtype: DISPATCH_TYPE.WORKER }); + return await env.JAEGER.enterSpan("dispatch_worker", async (span) => { + span.setTags({ + hasUserWorker: true, + asset: "ignored", + dispatchType: DISPATCH_TYPE.WORKER, + }); + + userWorkerInvocation = true; return env.USER_WORKER.fetch(maybeSecondRequest); }); } - // Otherwise, we try to first fetch assets, falling back to user-Worker. - if (env.CONFIG.has_user_worker) { - return env.JAEGER.enterSpan("has_user_worker", async (span) => { - if (await env.ASSET_WORKER.unstable_canFetch(request)) { - span.setTags({ - asset: true, - dispatchType: DISPATCH_TYPE.ASSETS, - }); - - analytics.setData({ dispatchtype: DISPATCH_TYPE.ASSETS }); - return env.ASSET_WORKER.fetch(maybeSecondRequest); - } else { - span.setTags({ - asset: false, - dispatchType: DISPATCH_TYPE.WORKER, - }); - - analytics.setData({ dispatchtype: DISPATCH_TYPE.WORKER }); - return env.USER_WORKER.fetch(maybeSecondRequest); - } + // If we have a user-Worker, but no assets, dispatch to Worker script + const assetsExist = await env.ASSET_WORKER.unstable_canFetch(request); + if (env.CONFIG.has_user_worker && !assetsExist) { + analytics.setData({ dispatchtype: DISPATCH_TYPE.WORKER }); + + return await env.JAEGER.enterSpan("dispatch_worker", async (span) => { + span.setTags({ + hasUserWorker: env.CONFIG.has_user_worker || false, + asset: assetsExist, + dispatchType: DISPATCH_TYPE.WORKER, + }); + + userWorkerInvocation = true; + return env.USER_WORKER.fetch(maybeSecondRequest); }); } - return env.JAEGER.enterSpan("assets_only", async (span) => { + // Otherwise, we either don't have a user worker, OR we have matching assets and should fetch from the assets binding + analytics.setData({ dispatchtype: DISPATCH_TYPE.ASSETS }); + return await env.JAEGER.enterSpan("dispatch_assets", async (span) => { span.setTags({ - asset: true, + hasUserWorker: env.CONFIG.has_user_worker || false, + asset: assetsExist, dispatchType: DISPATCH_TYPE.ASSETS, }); - analytics.setData({ dispatchtype: DISPATCH_TYPE.ASSETS }); - return env.ASSET_WORKER.fetch(request); + return env.ASSET_WORKER.fetch(maybeSecondRequest); }); } catch (err) { - if (err instanceof Error) { + if (userWorkerInvocation) { + // Don't send user Worker errors to sentry; we have no way to distinguish between + // CF errors and errors from the user's code. + return; + } else if (err instanceof Error) { analytics.setData({ error: err.message }); } diff --git a/packages/workers-shared/asset-worker/src/responses.ts b/packages/workers-shared/utils/responses.ts similarity index 100% rename from packages/workers-shared/asset-worker/src/responses.ts rename to packages/workers-shared/utils/responses.ts