Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Opting in via `experimentalUseDiagnosticsChannelInjection()` (before `init`)
// is all that's needed.
//
// `Sentry.init()` swaps the OTel `lru-memoizer` instrumentation
// for the diagnostics-channel one and synchronously
// installs the module hooks that inject the channels.
import * as Sentry from '@sentry/node';
import { loggingTransport } from '@sentry-internal/node-integration-tests';

Sentry.experimentalUseDiagnosticsChannelInjection();

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
release: '1.0',
tracesSampleRate: 1.0,
transport: loggingTransport,
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,51 +7,97 @@ describe('lru-memoizer', () => {
cleanupChildProcesses();
});

createEsmAndCjsTests(
__dirname,
'scenario.mjs',
'instrument.mjs',
(createTestRunner, test) => {
test('keeps outer context inside the memoized inner functions', async () => {
await createTestRunner()
.expect({
// Each case maps to the OpenTelemetry default and the diagnostics-channel opt-in
// variants, mirroring the mysql suite. `flags` are extra Node CLI flags; the
// instrument file is always loaded via `--import` (esm) / `--require` (cjs).
//
// lru-memoizer creates no spans, so there's no `sentry.origin` to
// assert: the opt-in cases prove the channel ran because the opt-in removes the
// OTel integration via `replacedOtelIntegrationNames`.
const CASES = [
// OpenTelemetry default — no opt-in, no injection. (OTel does not support ESM.)
{ label: 'opentelemetry (default)', instrument: 'instrument.mjs', flags: [], failsOnEsm: true },
// Opt-in via init only. `Sentry.init()` injects the channels synchronously.
{
label: 'diagnostics-channel (init opt-in)',
instrument: 'instrument-orchestrion.mjs',
flags: [],
failsOnEsm: false,
},
// Opt-in and rely on `node --import @sentry/node/import`.
{
label: 'diagnostics-channel (--import @sentry/node/import opt-in)',
instrument: 'instrument-orchestrion.mjs',
flags: ['--import', '@sentry/node/import'],
failsOnEsm: false,
},
// Without opt-in: channels are injected unconditionally but not subscribed to,
// so the OTel instrumentation does the work — proves injecting the channels has
// no downside. (OTel does not support ESM.)
{
label: 'opentelemetry (channels injected, no opt-in)',
instrument: 'instrument.mjs',
flags: ['--import', '@sentry/node/import'],
failsOnEsm: true,
},
] as const;

for (const { label, instrument, flags, failsOnEsm } of CASES) {
describe(label, () => {
createEsmAndCjsTests(
__dirname,
'scenario.mjs',
instrument,
(createTestRunner, test) => {
test('keeps outer context inside the memoized inner functions', async () => {
await createTestRunner()
.withFlags(...flags)
.expect({
transaction: {
transaction: '<unknown>',
contexts: {
trace: expect.objectContaining({
op: 'run',
data: expect.objectContaining({
'sentry.op': 'run',
'sentry.origin': 'manual',
'memoized.context_preserved': true,
}),
}),
},
},
})
.start()
.completed();
});
},
{ failsOnEsm },
);

// CJS-only: the parallel scenario is flaky in ESM (see #21729).
createCjsTests(__dirname, 'scenario-parallel.mjs', instrument, (createTestRunner, test) => {
test('keeps each span context across parallel memoized requests', async () => {
// Each parallel request emits a transaction whose callback must have run in its own context.
// Two identical expectations keep this order-independent.
const expectation = {
transaction: {
transaction: '<unknown>',
contexts: {
trace: expect.objectContaining({
op: 'run',
data: expect.objectContaining({
'sentry.op': 'run',
'sentry.origin': 'manual',
'memoized.context_preserved': true,
}),
op: expect.stringMatching(/^(first|second)$/),
data: expect.objectContaining({ 'memoized.context_preserved': true }),
}),
},
},
})
.start()
.completed();
});
},
{ failsOnEsm: true },
);

createCjsTests(__dirname, 'scenario-parallel.mjs', 'instrument.mjs', (createTestRunner, test) => {
test('keeps each span context across parallel memoized requests', async () => {
// Each parallel request emits a transaction whose callback must have run in its own context.
// Two identical expectations keep this order-independent.
const expectation = {
transaction: {
contexts: {
trace: expect.objectContaining({
op: expect.stringMatching(/^(first|second)$/),
data: expect.objectContaining({ 'memoized.context_preserved': true }),
}),
},
},
};
};

await createTestRunner().expect(expectation).expect(expectation).start().completed();
await createTestRunner()
.withFlags(...flags)
.expect(expectation)
.expect(expectation)
.start()
.completed();
});
});
});
});
}
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { mysqlChannelIntegration, detectOrchestrionSetup } from '@sentry/server-utils/orchestrion';
import {
mysqlChannelIntegration,
lruMemoizerChannelIntegration,
detectOrchestrionSetup,
} from '@sentry/server-utils/orchestrion';
import { registerDiagnosticsChannelInjection } from '@sentry/server-utils/orchestrion/register';
import type { DiagnosticsChannelInjection } from './diagnosticsChannelInjection';
import { setDiagnosticsChannelInjectionLoader } from './diagnosticsChannelInjection';
Expand Down Expand Up @@ -38,8 +42,8 @@ import { setDiagnosticsChannelInjectionLoader } from './diagnosticsChannelInject
export function experimentalUseDiagnosticsChannelInjection(): void {
setDiagnosticsChannelInjectionLoader(
(): DiagnosticsChannelInjection => ({
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

register: registerDiagnosticsChannelInjection,
detect: detectOrchestrionSetup,
}),
Comment thread
nicohrubec marked this conversation as resolved.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import * as diagnosticsChannel from 'node:diagnostics_channel';
import type { IntegrationFn } from '@sentry/core';
import { debug, defineIntegration, getCurrentScope, withScope } from '@sentry/core';
import { DEBUG_BUILD } from '../../debug-build';
import { CHANNELS } from '../../orchestrion/channels';

// Same name as the OTel integration by design — when enabled, the OTel
// 'LruMemoizer' integration is omitted from the default set.
const INTEGRATION_NAME = 'LruMemoizer';

interface LruMemoizerChannelContext {
arguments: unknown[];
}

const _lruMemoizerChannelIntegration = (() => {
return {
name: INTEGRATION_NAME,
setupOnce() {
// `tracingChannel` is unavailable before Node 18.19 — no-op instead of throwing (#21783).
if (!diagnosticsChannel.tracingChannel) {
return;
}
Comment thread
nicohrubec marked this conversation as resolved.

DEBUG_BUILD && debug.log(`[orchestrion:lru-memoizer] subscribing to channel "${CHANNELS.LRU_MEMOIZER_LOAD}"`);
const lruMemoizerCh = diagnosticsChannel.tracingChannel(CHANNELS.LRU_MEMOIZER_LOAD);

lruMemoizerCh.subscribe({
Comment thread
nicohrubec marked this conversation as resolved.
start(rawCtx) {
const ctx = rawCtx as LruMemoizerChannelContext;
if (ctx.arguments.length === 0) {
return;
}

// Capture the scope while we're still synchronously inside the memoized call.
// lru-memoizer queues the callback and fires it later via setImmediate, where the
// active scope no longer reflects the caller's context.
const scope = getCurrentScope();
const cbIdx = ctx.arguments.length - 1;
const orchestrionWrappedCb = ctx.arguments[cbIdx];

if (typeof orchestrionWrappedCb !== 'function') {
return;
}

const wrapped = orchestrionWrappedCb as (...a: unknown[]) => unknown;
ctx.arguments[cbIdx] = function (this: unknown, ...args: unknown[]): unknown {
return withScope(scope, () => wrapped.apply(this, args));
};
Comment on lines +45 to +48

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();
  });
}));

},
end() {},
asyncStart() {},
asyncEnd() {},
error() {},
});
},
};
}) satisfies IntegrationFn;

/**
* EXPERIMENTAL — orchestrion-driven lru-memoizer integration. Subscribes to
* `orchestrion:lru-memoizer:load` (injected into `lru-memoizer/lib/async.js`'s
* `memoizedFunction`). Creates no spans; only re-runs the memoized callback with the
* caller's scope. Requires the orchestrion runtime hook or bundler plugin.
*/
export const lruMemoizerChannelIntegration = defineIntegration(_lruMemoizerChannelIntegration);
1 change: 1 addition & 0 deletions packages/server-utils/src/orchestrion/channels.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
export const CHANNELS = {
MYSQL_QUERY: 'orchestrion:mysql:query',
LRU_MEMOIZER_LOAD: 'orchestrion:lru-memoizer:load',
} as const;

export type ChannelName = (typeof CHANNELS)[keyof typeof CHANNELS];
6 changes: 6 additions & 0 deletions packages/server-utils/src/orchestrion/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ export const SENTRY_INSTRUMENTATIONS: InstrumentationConfig[] = [
// attach `'end'`/`'error'` listeners that finish the span.
functionQuery: { expressionName: 'query', kind: 'Auto' },
},
{
channelName: 'load',
// `>=2.1.0` only: the named `function memoizedFunction()` the selector targets exists from 2.1.0
module: { name: 'lru-memoizer', versionRange: '>=2.1.0 <4', filePath: 'lib/async.js' },
functionQuery: { functionName: 'memoizedFunction', kind: 'Callback' },
},
];

/**
Expand Down
1 change: 1 addition & 0 deletions packages/server-utils/src/orchestrion/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export { detectOrchestrionSetup } from './detect';
export { mysqlChannelIntegration } from '../integrations/tracing-channel/mysql';
export { lruMemoizerChannelIntegration } from '../integrations/tracing-channel/lru-memoizer';
Loading