-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: start adding support to cf pages #129
base: main
Are you sure you want to change the base?
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,85 @@ | ||||
import { ReadableSpan } from '@opentelemetry/sdk-trace-base' | ||||
import { Initialiser, setConfig } from '../config' | ||||
import { exportSpans, proxyExecutionContext } from './common' | ||||
import { Exception, SpanKind, SpanOptions, SpanStatusCode, context as api_context, trace } from '@opentelemetry/api' | ||||
import { wrap } from '../wrap' | ||||
import { | ||||
gatherIncomingCfAttributes, | ||||
gatherRequestAttributes, | ||||
gatherResponseAttributes, | ||||
getParentContextFromRequest, | ||||
} from './fetch' | ||||
|
||||
type PageHandlerArgs = Parameters<PagesFunction> | ||||
|
||||
let cold_start = true | ||||
export function executePageHandler(pagesFn: PagesFunction, [request]: PageHandlerArgs): Promise<Response> { | ||||
const spanContext = getParentContextFromRequest(request.request) | ||||
|
||||
const tracer = trace.getTracer('pagesHandler') | ||||
const attributes = { | ||||
['faas.trigger']: 'http', | ||||
['faas.coldstart']: cold_start, | ||||
['faas.invocation_id']: request.request.headers.get('cf-ray') ?? undefined, | ||||
Comment on lines
+21
to
+23
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. Could you use semantic attributes import to remain consistent with the rest of the library? Once we update to the latest api, we will be able to tree-shake it to just these three keys being imported. |
||||
} | ||||
cold_start = false | ||||
Object.assign(attributes, gatherRequestAttributes(request.request)) | ||||
Object.assign(attributes, gatherIncomingCfAttributes(request.request)) | ||||
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. Could you also assign version metadata, like in otel-cf-workers/src/instrumentation/fetch.ts Line 144 in 3bdda1e
I know that version metadata bindings can't be added to Pages projects (looking at you @jahands), and script versions work very differently, but hopefully this will change as they converge more. |
||||
const options: SpanOptions = { | ||||
attributes, | ||||
kind: SpanKind.SERVER, | ||||
} | ||||
|
||||
const promise = tracer.startActiveSpan( | ||||
`${request.request.method} ${request.functionPath}`, | ||||
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. We recently introduced a new convention for instrumented spans, could you prefix this and the two |
||||
options, | ||||
spanContext, | ||||
async (span) => { | ||||
const readable = span as unknown as ReadableSpan | ||||
try { | ||||
const response: Response = await pagesFn(request) | ||||
span.setAttributes(gatherResponseAttributes(response)) | ||||
if (readable.attributes['http.route']) { | ||||
span.updateName(`${request.request.method} ${readable.attributes['http.route']}`) | ||||
} | ||||
span.end() | ||||
|
||||
return response | ||||
} catch (error) { | ||||
if (readable.attributes['http.route']) { | ||||
span.updateName(`${request.request.method} ${readable.attributes['http.route']}`) | ||||
} | ||||
span.recordException(error as Exception) | ||||
span.setStatus({ code: SpanStatusCode.ERROR }) | ||||
span.end() | ||||
throw error | ||||
} | ||||
}, | ||||
) | ||||
return promise | ||||
} | ||||
|
||||
export function createPageHandler< | ||||
E = unknown, | ||||
P extends string = any, | ||||
D extends Record<string, unknown> = Record<string, unknown>, | ||||
>(pageFn: PagesFunction<E, P, D>, initialiser: Initialiser): PagesFunction<E, P, D> { | ||||
const pagesHandler: ProxyHandler<PagesFunction> = { | ||||
apply: async (target, _thisArg, argArray: Parameters<PagesFunction>): Promise<Response> => { | ||||
const [orig_ctx] = argArray | ||||
const config = initialiser(orig_ctx.env as Record<string, unknown>, orig_ctx.request) | ||||
const { ctx, tracker } = proxyExecutionContext(orig_ctx) | ||||
const context = setConfig(config) | ||||
|
||||
try { | ||||
const args: PageHandlerArgs = [ctx] as PageHandlerArgs | ||||
return await api_context.with(context, executePageHandler, undefined, target, args) | ||||
} catch (error) { | ||||
throw error | ||||
} finally { | ||||
orig_ctx.waitUntil(exportSpans(tracker)) | ||||
} | ||||
}, | ||||
} | ||||
return wrap(pageFn, pagesHandler) | ||||
} |
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.
We should make this a bit longer to help ensure it doesn't ever conflict with user code (generally, global vars should be avoided in Workers/Functions, but this seems ok.)
e.g.
let __otel_cf_is_cold_start = true
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 think this should actually be fine, as it's not exported, so user code can never find it as
cold_start
.