feat(server-utils): Add lru-memoizer diagnostics-channel integration#21786
feat(server-utils): Add lru-memoizer diagnostics-channel integration#21786nicohrubec wants to merge 3 commits into
Conversation
02dfa93 to
86b7148
Compare
Adds an experimental orchestrion (diagnostics-channel) integration for lru-memoizer, active only when `experimentalUseDiagnosticsChannelInjection()` is opted into. It rebinds the memoized callback to the caller's active span (the channel equivalent of the OTel `context.bind`) and creates no spans. Purely additive — the vendored OTel integration stays the default and is filtered out via `replacedOtelIntegrationNames` only when opted in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 783398f. Configure here.
| integrations: [mysqlChannelIntegration()], | ||
| replacedOtelIntegrationNames: ['Mysql'], | ||
| integrations: [mysqlChannelIntegration(), lruMemoizerChannelIntegration()], | ||
| replacedOtelIntegrationNames: ['Mysql', 'LruMemoizer'], |
There was a problem hiding this comment.
Opt-in drops older lru-memoizer
Medium Severity
Calling experimentalUseDiagnosticsChannelInjection() removes the OpenTelemetry LruMemoizer integration, but orchestrion injection only targets lru-memoizer >=2.1.0. Apps on 1.3.x–2.0.x that opt in lose callback context binding with no channel replacement.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 783398f. Configure here.
There was a problem hiding this comment.
<2.1.0 is barely used anymore so I think this should be fine: https://www.npmjs.com/package/lru-memoizer?activeTab=versions
| const wrapped = orchestrionWrappedCb as (...a: unknown[]) => unknown; | ||
| ctx.arguments[cbIdx] = function (this: unknown, ...args: unknown[]): unknown { | ||
| return withActiveSpan(parentSpan, () => wrapped.apply(this, args)); | ||
| }; |
There was a problem hiding this comment.
I think we should capture the whole scope, mysql integration seems to do that and is very similar to this.
| const wrapped = orchestrionWrappedCb as (...a: unknown[]) => unknown; | |
| ctx.arguments[cbIdx] = function (this: unknown, ...args: unknown[]): unknown { | |
| return withActiveSpan(parentSpan, () => wrapped.apply(this, args)); | |
| }; | |
| const scope = getCurrentScope(); | |
| const wrapped = orchestrionWrappedCb as (...a: unknown[]) => unknown; | |
| ctx.arguments[cbIdx] = function (this: unknown, ...args: unknown[]): unknown { | |
| return withScope(scope, () => wrapped.apply(this, args)); | |
| }; |
There was a problem hiding this comment.
I couldn't reproduce a case locally where it actually made a difference but I aligned it with mysql
There was a problem hiding this comment.
It wouldn't make a difference with spans I think, but tags and other stuff maybe. Claude came up with this test:
import * as Sentry from '@sentry/node';
import memoizer from 'lru-memoizer';
let fireLoad;
const memoized = memoizer({
load: (_arg, cb) => { fireLoad = cb; }, // capture callback, don't resolve yet
hash: () => 'key',
});
Sentry.startSpan({ name: 'caller' }, span => new Promise(resolve => {
Sentry.getCurrentScope().setTag('tag', 'from-caller'); // set on caller scope
memoized({ foo: 'bar' }, () => {
const tags = Sentry.getCurrentScope().getScopeData().tags;
console.log('active span preserved:', Sentry.getActiveSpan()?.spanContext().spanId === span.spanContext().spanId);
console.log('tag preserved: ', tags.tag === 'from-caller');
resolve();
});
}));…hannel subscriber Aligns the lru-memoizer diagnostics-channel subscriber with the mysql one: capture `getCurrentScope()` and re-run the memoized callback via `withScope(scope, …)` instead of `withActiveSpan(getActiveSpan(), …)`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>


Adds an orchestrion-based lru-memoizer integration replacing the equivalent otel integration if enabled. This currently supports
>=2.1.0, which should be sufficient I think since<2.1.0is barely used anymore.Closes #20759