-
-
Notifications
You must be signed in to change notification settings - Fork 367
feat(tracing): sync JS scope propagation context to native scope #6686
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: main
Are you sure you want to change the base?
Changes from all commits
804afe6
b452c98
2fe54fd
944c179
4de5005
ae93384
a4203fb
5590a26
9c65ce8
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 |
|---|---|---|
|
|
@@ -6,19 +6,21 @@ | |
| getActiveSpan, | ||
| getClient, | ||
| getCurrentScope, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
| SentryNonRecordingSpan, | ||
| spanIsSampled, | ||
| SPAN_STATUS_ERROR, | ||
| spanToJSON, | ||
| startIdleSpan as coreStartIdleSpan, | ||
| } from '@sentry/core'; | ||
| import { AppState, Platform } from 'react-native'; | ||
|
|
||
| import { isRootSpan } from '../utils/span'; | ||
| import { NATIVE } from '../wrapper'; | ||
| import { adjustTransactionDuration, cancelInBackground } from './onSpanEndUtils'; | ||
| import { | ||
| SPAN_ORIGIN_AUTO_INTERACTION, | ||
|
Check warning on line 23 in packages/core/src/js/tracing/span.ts
|
||
| SPAN_ORIGIN_AUTO_NAVIGATION_CUSTOM, | ||
| SPAN_ORIGIN_MANUAL_INTERACTION, | ||
| } from './origin'; | ||
|
|
@@ -202,3 +204,29 @@ | |
| spanJSON.data[SPAN_THREAD_NAME] = SPAN_THREAD_NAME_MAIN; | ||
| return spanJSON; | ||
| } | ||
|
|
||
| /** | ||
| * Pushes the JS root span's propagation context to the native scope so that | ||
| * native HTTP instrumentation (OkHttp on Android, URLSession on iOS) attaches | ||
| * the correct traceId and appears in the same trace as the JS transaction. | ||
| * | ||
| * Fires for every root span so that the native scope is always up to date and | ||
| * doesn't retain a stale traceId from a previous span. Child spans are skipped | ||
| * to avoid bridge spam. | ||
| * | ||
| * Note: `spanStart` fires before idle spans are made active, so an active-span | ||
| * guard here would skip every navigation span. Sync happens for all root spans. | ||
| */ | ||
| export function syncPropagationContextToNative(client: Client): void { | ||
| client.on('spanStart', (span: Span) => { | ||
| if (!isRootSpan(span)) return; | ||
| const ctx = span.spanContext(); | ||
| const propagationCtx = getCurrentScope().getPropagationContext(); | ||
| NATIVE.setCurrentScopePropagationContext({ | ||
| traceId: ctx.traceId, | ||
| spanId: ctx.spanId, | ||
| sampled: spanIsSampled(span), | ||
| sampleRand: propagationCtx.sampleRand ?? Math.random(), | ||
|
Comment on lines
+223
to
+229
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. Bug: Root spans created without Suggested FixEnsure that code paths creating root spans, like Prompt for AI Agent
Contributor
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. This looks valid ๐ |
||
| }); | ||
|
sentry[bot] marked this conversation as resolved.
|
||
| }); | ||
|
Check warning on line 231 in packages/core/src/js/tracing/span.ts
|
||
|
sentry-warden[bot] marked this conversation as resolved.
|
||
| } | ||
|
cursor[bot] marked this conversation as resolved.
alwx marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import type { Client } from '@sentry/core'; | ||
|
|
||
| import { getCurrentScope, SentryNonRecordingSpan, startInactiveSpan } from '@sentry/core'; | ||
|
|
||
| jest.mock('../../src/js/wrapper', () => ({ | ||
| NATIVE: { | ||
| enableNative: true, | ||
| setCurrentScopePropagationContext: jest.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('react-native', () => ({ | ||
| AppState: { | ||
| currentState: 'active', | ||
| addEventListener: jest.fn(() => ({ remove: jest.fn() })), | ||
| }, | ||
| Platform: { OS: 'ios' }, | ||
| NativeModules: { RNSentry: {} }, | ||
| })); | ||
|
|
||
| import { syncPropagationContextToNative } from '../../src/js/tracing/span'; | ||
| import { NATIVE } from '../../src/js/wrapper'; | ||
| import { setupTestClient } from '../mocks/client'; | ||
|
|
||
| const mockSetPropagationContext = NATIVE.setCurrentScopePropagationContext as jest.Mock; | ||
|
|
||
| describe('syncPropagationContextToNative', () => { | ||
| let client: Client; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| client = setupTestClient({ tracesSampleRate: 1.0 }); | ||
| syncPropagationContextToNative(client); | ||
| }); | ||
|
|
||
| it('calls NATIVE.setCurrentScopePropagationContext when a root span starts', () => { | ||
| const span = startInactiveSpan({ name: 'root', forceTransaction: true }); | ||
| const ctx = span.spanContext(); | ||
|
|
||
| expect(mockSetPropagationContext).toHaveBeenCalledTimes(1); | ||
| expect(mockSetPropagationContext).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| traceId: ctx.traceId, | ||
| spanId: ctx.spanId, | ||
| }), | ||
| ); | ||
|
|
||
| span.end(); | ||
| }); | ||
|
|
||
| it('does not call NATIVE.setCurrentScopePropagationContext for child spans', () => { | ||
| const root = startInactiveSpan({ name: 'root', forceTransaction: true }); | ||
| mockSetPropagationContext.mockClear(); | ||
|
|
||
| const child = startInactiveSpan({ name: 'child', parentSpan: root }); | ||
| expect(mockSetPropagationContext).not.toHaveBeenCalled(); | ||
|
|
||
| child.end(); | ||
| root.end(); | ||
| }); | ||
|
|
||
| it('calls NATIVE.setCurrentScopePropagationContext for SentryNonRecordingSpan to prevent stale native context', () => { | ||
| const nonRecording = new SentryNonRecordingSpan(); | ||
| const ctx = nonRecording.spanContext(); | ||
| client.emit('spanStart', nonRecording); | ||
|
|
||
| expect(mockSetPropagationContext).toHaveBeenCalledTimes(1); | ||
| expect(mockSetPropagationContext).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| traceId: ctx.traceId, | ||
| spanId: ctx.spanId, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('passes sampled and sampleRand from the scope propagation context', () => { | ||
| const span = startInactiveSpan({ name: 'root', forceTransaction: true }); | ||
| const propagationCtx = getCurrentScope().getPropagationContext(); | ||
|
|
||
| expect(mockSetPropagationContext).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| sampled: expect.any(Boolean), | ||
| sampleRand: propagationCtx.sampleRand ?? expect.any(Number), | ||
| }), | ||
| ); | ||
|
|
||
| span.end(); | ||
| }); | ||
|
|
||
| it('fires once per root span start', () => { | ||
| const span1 = startInactiveSpan({ name: 'first', forceTransaction: true }); | ||
| span1.end(); | ||
|
|
||
| const span2 = startInactiveSpan({ name: 'second', forceTransaction: true }); | ||
| span2.end(); | ||
|
|
||
| expect(mockSetPropagationContext).toHaveBeenCalledTimes(2); | ||
| expect(mockSetPropagationContext.mock.calls[0][0].spanId).toBe(span1.spanContext().spanId); | ||
| expect(mockSetPropagationContext.mock.calls[1][0].spanId).toBe(span2.spanContext().spanId); | ||
| }); | ||
| }); |
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.
q: Why do we change this? I think it might degrade the performance