feat(server-utils): Add @opentelemetry/instrumentation-koa orchestrion integration#22146
feat(server-utils): Add @opentelemetry/instrumentation-koa orchestrion integration#22146s1gr1d wants to merge 2 commits into
Conversation
s1gr1d
requested review from
JPeer264,
andreiborza and
mydea
and removed request for
a team
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
| 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.
| // 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.
| }; | ||
| } | ||
|
|
||
| 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.
2 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 - check todo comments).
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