Skip to content

feat(server-utils): Add lru-memoizer diagnostics-channel integration#21786

Open
nicohrubec wants to merge 3 commits into
developfrom
nh/orchestrion-lru-memoizer
Open

feat(server-utils): Add lru-memoizer diagnostics-channel integration#21786
nicohrubec wants to merge 3 commits into
developfrom
nh/orchestrion-lru-memoizer

Conversation

@nicohrubec

@nicohrubec nicohrubec commented Jun 25, 2026

Copy link
Copy Markdown
Member

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.0 is barely used anymore.

Closes #20759

@nicohrubec nicohrubec force-pushed the nh/orchestrion-lru-memoizer branch from 02dfa93 to 86b7148 Compare June 25, 2026 12:59
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>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ 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'],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.x2.0.x that opt in lose callback context binding with no channel replacement.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 783398f. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

<2.1.0 is barely used anymore so I think this should be fine: https://www.npmjs.com/package/lru-memoizer?activeTab=versions

@nicohrubec nicohrubec marked this pull request as ready for review June 26, 2026 12:19
@nicohrubec nicohrubec requested a review from a team as a code owner June 26, 2026 12:19
@nicohrubec nicohrubec requested review from JPeer264, andreiborza and mydea and removed request for a team June 26, 2026 12:19
Comment thread packages/server-utils/src/integrations/tracing-channel/lru-memoizer.ts Outdated
@nicohrubec nicohrubec requested a review from logaretm June 26, 2026 14:02
Comment on lines +45 to +48
const wrapped = orchestrionWrappedCb as (...a: unknown[]) => unknown;
ctx.arguments[cbIdx] = function (this: unknown, ...args: unknown[]): unknown {
return withActiveSpan(parentSpan, () => wrapped.apply(this, args));
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should capture the whole scope, mysql integration seems to do that and is very similar to this.

Suggested change
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));
};

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I couldn't reproduce a case locally where it actually made a difference but I aligned it with mysql

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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>
@nicohrubec nicohrubec requested a review from logaretm June 26, 2026 19:07
Comment thread packages/node/src/sdk/experimentalUseDiagnosticsChannelInjection.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rewrite @opentelemetry/instrumentation-lru-memoizer to orchestrion

2 participants