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

Fix: Router-worker loss of sentry + metrics on error #7906

Merged
merged 1 commit into from
Jan 27, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/olive-tigers-raise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@cloudflare/workers-shared": minor
---

Fixes bug in router-worker that prevents unexpected errors from being captured.
2 changes: 1 addition & 1 deletion packages/workers-shared/asset-worker/src/handler.ts
Original file line number Diff line number Diff line change
@@ -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";
8 changes: 3 additions & 5 deletions packages/workers-shared/asset-worker/src/index.ts
Original file line number Diff line number Diff line change
@@ -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<Env> {
});
}

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<Env> {
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);
9 changes: 8 additions & 1 deletion packages/workers-shared/router-worker/src/analytics.ts
Original file line number Diff line number Diff line change
@@ -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,
72 changes: 41 additions & 31 deletions packages/workers-shared/router-worker/src/index.ts
Original file line number Diff line number Diff line change
@@ -30,6 +30,7 @@ interface Env {
export default {
async fetch(request: Request, env: Env, ctx: ExecutionContext) {
let sentry: ReturnType<typeof setupSentry> | 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) {
WillTaylorDev marked this conversation as resolved.
Show resolved Hide resolved
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)) {
WillTaylorDev marked this conversation as resolved.
Show resolved Hide resolved
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) => {
WillTaylorDev marked this conversation as resolved.
Show resolved Hide resolved
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 });
}

Loading