feat: start adding support to cf pages#129
feat: start adding support to cf pages#129vanstinator wants to merge 2 commits intoevanderkoogh:mainfrom
Conversation
6b0890e to
ecb4583
Compare
|
|
||
| type PageHandlerArgs = Parameters<PagesFunction> | ||
|
|
||
| let cold_start = true |
There was a problem hiding this comment.
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.
I think this should actually be fine, as it's not exported, so user code can never find it as cold_start.
| ['faas.trigger']: 'http', | ||
| ['faas.coldstart']: cold_start, | ||
| ['faas.invocation_id']: request.request.headers.get('cf-ray') ?? undefined, |
There was a problem hiding this comment.
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.
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 promise = tracer.startActiveSpan( | ||
| `${request.request.method} ${request.functionPath}`, |
There was a problem hiding this comment.
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.
|
Hey this would be really useful for a project I am working on. Is this usable as is? Are there plans to merge this? |
|
@DaniFoldi @jahands @evanderkoogh This feature is badly needed/wanted. Can let us know about any blockers to merging this PR? We're nearing 1 year since it was opened. I'm happy to open a new PR for this and address feedback if that helps. |
|
Hey, I hate to bring bad news, but from my side, I don't actually have merge perms on the repo, and with my team we've created a different otel library (not opensource, as it relies on other internal libs, unfortunately) so I haven't really worked on my fork in a while. |
|
I'm no longer with the company I was with back when I opened this, and I don't have a cf-pages project of my own to play with. So I'm afraid I won't be able to take this one over the finish line myself. |
|
@evanderkoogh Now that the core-logic-refactor has landed can you take a look at adapting this to the new approach, or explaining how to adapt it? Anecdotally, this PR has worked well for me in instrumenting two Pages sites |
|
Hey @jfsiii. I am currently looking into fixing RPC style communication. As that is one of the last things that is not working in currently supported modules. But this PR should be relatively straightforward to put into the otel-cf-workers/src/instrumentation/fetch.ts Lines 133 to 157 in 289a7cd Then it is probably doing something extremely similar to otel-cf-workers/src/instrumentation/fetch.ts Lines 133 to 157 in 289a7cd Pages is probably the next thing on my agenda after the RPC thing, but the RPC thing isn't going to be super trivial unfortunately :( |
|
@evanderkoogh Thanks! Let me know if I can help build/test RPC. We use it extensively in our project and I'm desperate to get OTEL tracing for it. |
|
What would be amazing @jfsiii, is if you could create some concise examples of either or both RPC and Pages projects in the |
|
@evanderkoogh I'm mid-sprint right now, but I can probably do that next week |
Fixes #96
What this PR solves / how to test:
This PR adds support for automatically instrumenting Cloudflare Pages. The code is very similar to the
fetchcode. I'd initially intended to combine the similar bits into a shared core, but as I started to get into those weeds I realized I didn't have a ton of context on the full set of differences between Pages and Workers, and it started to seem like keeping the code paths separate would allow for a more organic growth on the Pages instrumentation.To test this code yourself it's as simple as wrapping a Cloudflare Pages handler in
instrumentPageand passing it a validTraceConfig.