-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(cloudflare): Instrument async KV API #19404
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import type { DurableObjectStorage } from '@cloudflare/workers-types'; | ||
| import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startSpan } from '@sentry/core'; | ||
|
|
||
| const STORAGE_METHODS_TO_INSTRUMENT = ['get', 'put', 'delete', 'list'] as const; | ||
|
|
||
| type StorageMethod = (typeof STORAGE_METHODS_TO_INSTRUMENT)[number]; | ||
|
|
||
| /** | ||
| * Instruments DurableObjectStorage methods with Sentry spans. | ||
| * | ||
| * Wraps the following async methods: | ||
| * - get, put, delete, list (KV API) | ||
| * | ||
| * @param storage - The DurableObjectStorage instance to instrument | ||
| * @returns An instrumented DurableObjectStorage instance | ||
| */ | ||
| export function instrumentDurableObjectStorage(storage: DurableObjectStorage): DurableObjectStorage { | ||
| return new Proxy(storage, { | ||
| get(target, prop, receiver) { | ||
| const original = Reflect.get(target, prop, receiver); | ||
|
|
||
| if (typeof original !== 'function') { | ||
| return original; | ||
| } | ||
|
|
||
| const methodName = prop as string; | ||
| if (!STORAGE_METHODS_TO_INSTRUMENT.includes(methodName as StorageMethod)) { | ||
| return (original as (...args: unknown[]) => unknown).bind(target); | ||
| } | ||
|
|
||
| return function (this: unknown, ...args: unknown[]) { | ||
| return startSpan( | ||
| { | ||
| // Use underscore naming to match Cloudflare's native instrumentation (e.g., "durable_object_storage_get") | ||
| name: `durable_object_storage_${methodName}`, | ||
| op: 'db', | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.db.cloudflare.durable_object', | ||
| 'db.system.name': 'cloudflare.durable_object.storage', | ||
| 'db.operation.name': methodName, | ||
| }, | ||
| }, | ||
| () => { | ||
| return (original as (...args: unknown[]) => unknown).apply(target, args); | ||
| }, | ||
| ); | ||
| }; | ||
| }, | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import type { DurableObjectStorage } from '@cloudflare/workers-types'; | ||
| import { | ||
| captureException, | ||
| flush, | ||
|
|
@@ -14,6 +15,11 @@ import type { CloudflareOptions } from './client'; | |
| import { isInstrumented, markAsInstrumented } from './instrument'; | ||
| import { init } from './sdk'; | ||
|
|
||
| /** Extended DurableObjectState with originalStorage exposed by instrumentContext */ | ||
| interface InstrumentedDurableObjectState extends DurableObjectState { | ||
| originalStorage?: DurableObjectStorage; | ||
| } | ||
|
|
||
| type MethodWrapperOptions = { | ||
| spanName?: string; | ||
| spanOp?: string; | ||
|
|
@@ -58,13 +64,13 @@ export function wrapMethodWithSentry<T extends OriginalMethod>( | |
| // In certain situations, the passed context can become undefined. | ||
| // For example, for Astro while prerendering pages at build time. | ||
| // see: https://github.com/getsentry/sentry-javascript/issues/13217 | ||
| const context = wrapperOptions.context as ExecutionContext | undefined; | ||
| const context = wrapperOptions.context as InstrumentedDurableObjectState | undefined; | ||
|
|
||
| const waitUntil = context?.waitUntil?.bind?.(context); | ||
|
|
||
| const currentClient = scope.getClient(); | ||
| if (!currentClient) { | ||
| const client = init({ ...wrapperOptions.options, ctx: context }); | ||
| const client = init({ ...wrapperOptions.options, ctx: context as unknown as ExecutionContext | undefined }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The flush lock mechanism is ineffective for Durable Objects because it relies on Suggested FixDetect if the context is a Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
| scope.setClient(client); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about caching this function and retrieving from cache? The underlying storage functions should never really change, and I think especially in a KV-storage we should consider perf implications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a task for this: #19451
I want to go deeper a little before making this decision