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

feat: start adding support to cf pages #129

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion src/instrumentation/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export function getParentContextFromHeaders(headers: Headers): Context {
})
}

function getParentContextFromRequest(request: Request) {
export function getParentContextFromRequest(request: Request) {
const workerConfig = getActiveConfig()
const acceptTraceContext =
typeof workerConfig.handlers.fetch.acceptTraceContext === 'function'
Expand Down
85 changes: 85 additions & 0 deletions src/instrumentation/page.ts
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
Copy link
Collaborator

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

Copy link
Contributor

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also assign version metadata, like in

Object.assign(attributes, versionAttributes(env))

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}`,
Copy link
Contributor

Choose a reason for hiding this comment

The 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 updateNames to be prefixed with fetchHandler? Again, we could call them functionHandler, but with convergence a consistent name between Workers and Pages will make more sense in my opinion.

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)
}
12 changes: 12 additions & 0 deletions src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,18 @@ function createInitialiser(config: ConfigurationOption): Initialiser {
}
}

export function instrumentPage<
E = unknown,
P extends string = any,
D extends Record<string, unknown> = Record<string, unknown>,
>(handler: PagesFunction<E, P, D>, config: ConfigurationOption): PagesFunction<E, P, D> {
const initialiser = createInitialiser(config)

handler = createPageHandler(handler, initialiser)

return handler
}

export function instrument<E, Q, C>(
handler: ExportedHandler<E, Q, C>,
config: ConfigurationOption,
Expand Down