-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(server-utils): Add lru-memoizer diagnostics-channel integration #21786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
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({ | ||||||||||||||||||||
|
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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||||||||||||||||||||
| 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'; |
There was a problem hiding this comment.
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 OpenTelemetryLruMemoizerintegration, but orchestrion injection only targetslru-memoizer>=2.1.0. Apps on1.3.x–2.0.xthat opt in lose callback context binding with no channel replacement.Additional Locations (1)
packages/server-utils/src/orchestrion/config.ts#L35-L39Reviewed by Cursor Bugbot for commit 783398f. Configure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<2.1.0is barely used anymore so I think this should be fine: https://www.npmjs.com/package/lru-memoizer?activeTab=versions