feat(server-utils): Add @opentelemetry/instrumentation-koa orchestrion integration#22146
Conversation
size-limit report 📦
|
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This looks good!
The main question is if the update to orchestrion can allow us to instrument koa's main export properly. Apart from that just some mechanical stuff and a few bikeshed nits. (Wire Deno's options through, and update test snapshots so the CI stops complaining.)
| * spans nest under the active HTTP server span. | ||
| */ | ||
| const _denoKoaIntegration = (() => { | ||
| const inner = koaChannelIntegration(); |
There was a problem hiding this comment.
m: This loses the ignoreLayersType option. It should take the options and thread them through, like how we do in packages/deno/src/integrations/postgres.ts
There was a problem hiding this comment.
This is still not threading the options through, but tbh, maybe it doesn't matter (re discussion this morning in platform team weekly). This is still more tracing integration than Koa has in Deno today, and once #22306 gets implemented, we might rip this out anyway. So, bottom line, 👍
| export const koaConfig = [ | ||
| { | ||
| channelName: 'compose', | ||
| module: { name: 'koa-compose', versionRange: '>=4.0.0 <5', filePath: 'index.js' }, |
There was a problem hiding this comment.
The rationale in the PR description makes sense, and this is a good workaround to address it.
But I believe that the require(esm) issue will be addressed by @apm-js-collab/tracing-hooks's next release, once we land apm-js-collab/tracing-hooks#45
Is that accurate? If so, it might be worth backing this out, and patching the main export directly, if only to keep the versions aligned with the OTel instrumentation.
There was a problem hiding this comment.
Yes, this PR would fix this! I think it's worth waiting for it and then patching the main export (it's more accurate)
| // subscription here. | ||
| let subscribed = false; | ||
|
|
||
| let ignoreLayersType: KoaLayerType[] = []; |
There was a problem hiding this comment.
nit: This is functionally fine, because of the subscriber/integration deduping. But we've done this sort of "integration-local" variables within a closure in eg pg/redis/ioredis.
I don't have a strong opinion about which is better, but it might be good to settle on one pattern. The closure approach seems like it might be slightly safer, because it's future-proofed against potentially relaxing the deduplication constraint, but it seems unlikely we'd ever do that, so this is very much 100% a bikeshed nit.
There was a problem hiding this comment.
yeah makes sense to settle on one pattern here (the closure approach).
| }; | ||
| } | ||
|
|
||
| function getMiddlewareMetadata( |
There was a problem hiding this comment.
Rather than duplicate all the stuff from the vendored utils.ts, it'd be good to extract those out to a shared helpers.ts first, just to have one less thing to keep in sync.
There was a problem hiding this comment.
Hm, on further thought, I guess that somewhat is tricky because the utils.ts file is in a completely different package (sentry/node vs sentry/server-utils) 🤔
I'm not sure if it makes sense to have the node SDK's vendored otel koa instrumentation pull from server-utils to get its reusables. Feels like a stretch. Maybe this concern can be addressed with a comment just mentioning that the two files should be kept in sync as long as we're using the otel koa in node. They're unlikely to change very much anyway, I suppose.
|
I will come back to this PR once apm-js-collab/tracing-hooks#45 is released - this gives us the possibility to patch koa like we did before (not just |
|
@s1gr1d please readd me as reviewer once this is ready to review again 🥳 |
# Conflicts: # packages/deno/src/index.ts # packages/server-utils/src/orchestrion/channels.ts # packages/server-utils/src/orchestrion/config/index.ts # yarn.lock
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
LGTM! A few comments still, but nothing blocking imo.
| channelName: 'use', | ||
| module: { name: 'koa', versionRange: '>=2.0.0 <4', filePath: 'lib/application.js' }, | ||
| functionQuery: { className: 'Application', methodName: 'use', kind: 'Sync' }, | ||
| }, |
| * spans nest under the active HTTP server span. | ||
| */ | ||
| const _denoKoaIntegration = (() => { | ||
| const inner = koaChannelIntegration(); |
There was a problem hiding this comment.
This is still not threading the options through, but tbh, maybe it doesn't matter (re discussion this morning in platform team weekly). This is still more tracing integration than Koa has in Deno today, and once #22306 gets implemented, we might rip this out anyway. So, bottom line, 👍
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
# Conflicts: # packages/deno/src/index.ts # packages/server-utils/package.json # yarn.lock
That was an oversight! I added it before merging this. |
3 participants
Adds
koaChannelIntegrationin@sentry/server-utilsfor injecting orchestrion channels intokoa.A subscriber wraps each registered layer in a span-creating proxy.
Span-helpers are ported from the vendored instrumentation, preserving span names (but adapting to new conventions).
Also upgraded
@apm-js-collab/tracing-hooksto get this: apm-js-collab/tracing-hooks#45 to be released (lets us actually patchkoa- see 1. iteration below).1. iteration
One thing to know for review: we instrument
koa-compose, notusefrom koa. koa'suselives in koa's main entry (lib/application.js), and transforming a package's main entry forces its top-levelrequirechain through Node'srequire(esm)bridge, which throws on Node < 24.13.uselives in koa's main entry (lib/application.js), so to instrument it we transform that file. But transforming a main entry pulls its whole top-levelrequirechain into the loader's handling --> and that changes how thoserequires are loaded (through the ESM→CommonJS translator, not the normal sync require path).koais CJS (but support ESM). Whenimporting koa, it loads a shim that loads the CJS code:import 'koa'→dist/koa.mjs(a ESM shim) →import '../lib/application.js'(CJS). That CJS entry has a top-levelrequire('is-generator-function').is-generator-function→require('generator-function'), andgenerator-functionpoints at an.mjsfile, which in turn imports./index.js.--> So the top-level
requirein koa'sapplication.js(CJS) becomesrequire(esm)of an ESM file importing a CJS file.require(esm)cache, so it throwsrequest for './index.js' is not in cache.The failing chain:
koa-composeis koa's zero-dependency dispatch engine, so it's safe to transform, andcompose(app.middleware)sees the same layersusewould. Since@koa/routeralso callscomposeper request, the subscriber usesgetActiveSpan()to only wrap at app startup (no active span) and skip the per-request router composition.Closes #20758
Linear: https://linear.app/getsentry/issue/JS-2409/rewrite-opentelemetryinstrumentation-koa-to-orchestrion