Skip to content

Commit

Permalink
Merge pull request #572 from bugsnag/PLAT-13443/fail-loudly-on-attach
Browse files Browse the repository at this point in the history
Throw errors from attach when native SDK is not available
  • Loading branch information
yousif-bugsnag authored Jan 22, 2025
2 parents 6c8c4bf + 132096e commit 9c595f0
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 51 deletions.
5 changes: 2 additions & 3 deletions packages/platforms/react-native/lib/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ const backgroundingListener = createBrowserBackgroundingListener(AppState)
const xhrRequestTracker = createXmlHttpRequestTracker(XMLHttpRequest, clock)

const idGenerator = createIdGenerator(isDebuggingRemotely)
const schema = createSchema()
const BugsnagPerformance = createClient({
backgroundingListener,
clock,
Expand All @@ -49,11 +48,11 @@ const BugsnagPerformance = createClient({
new NetworkRequestPlugin(spanFactory, spanContextStorage, xhrRequestTracker)
],
resourceAttributesSource,
schema,
schema: createSchema(),
spanAttributesSource,
retryQueueFactory: createRetryQueueFactory(FileSystem),
spanFactory: ReactNativeSpanFactory,
platformExtensions: (spanFactory, spanContextStorage) => platformExtensions(appStartTime, clock, schema, spanFactory as ReactNativeSpanFactory, spanContextStorage)
platformExtensions: (spanFactory, spanContextStorage) => platformExtensions(appStartTime, clock, spanFactory as ReactNativeSpanFactory, spanContextStorage)
})

export default BugsnagPerformance
11 changes: 4 additions & 7 deletions packages/platforms/react-native/lib/platform-extensions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import type { Client, Clock, SpanContextStorage, SpanOptions } from '@bugsnag/co
import React from 'react'
import { Platform } from 'react-native'
import NativeBugsnagPerformance from './native'
import type { ReactNativeAttachConfiguration, ReactNativeConfiguration, ReactNativeSchema } from './config'
import type { ReactNativeAttachConfiguration, ReactNativeConfiguration } from './config'
import { createAppStartSpan } from './create-app-start-span'
import type { ReactNativeSpanFactory } from './span-factory'

type NavigationSpanOptions = Omit<SpanOptions, 'isFirstClass'>

export const platformExtensions = (appStartTime: number, clock: Clock, schema: ReactNativeSchema, spanFactory: ReactNativeSpanFactory, spanContextStorage: SpanContextStorage) => ({
export const platformExtensions = (appStartTime: number, clock: Clock, spanFactory: ReactNativeSpanFactory, spanContextStorage: SpanContextStorage) => ({
startNavigationSpan: function (routeName: string, spanOptions?: NavigationSpanOptions) {
const cleanOptions = spanFactory.validateSpanOptions(routeName, spanOptions)
const span = spanFactory.startNavigationSpan(cleanOptions.name, cleanOptions.options)
Expand All @@ -28,18 +28,15 @@ export const platformExtensions = (appStartTime: number, clock: Clock, schema: R
}
},
attach: function (config?: ReactNativeAttachConfiguration) {
const logger = schema.logger.validate(config?.logger) ? config.logger : schema.logger.defaultValue
const platform = Platform.OS === 'ios' ? 'Cocoa' : 'Android'
const isNativePerformanceAvailable = NativeBugsnagPerformance?.isNativePerformanceAvailable()
if (!isNativePerformanceAvailable) {
logger.warn(`Could not attach to native SDK. No compatible version of Bugsnag ${platform} Performance was found.`)
return
throw new Error(`Could not attach to native SDK. No compatible version of Bugsnag ${platform} Performance was found.`)
}

const nativeConfig = NativeBugsnagPerformance?.attachToNativeSDK()
if (!nativeConfig) {
logger.warn(`Could not attach to native SDK. Bugsnag ${platform} Performance has not been started.`)
return
throw new Error(`Could not attach to native SDK. Bugsnag ${platform} Performance has not been started.`)
}

const finalConfig: ReactNativeConfiguration = {
Expand Down
8 changes: 4 additions & 4 deletions packages/platforms/react-native/lib/span-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export class ReactNativeSpanFactory extends SpanFactory<ReactNativeConfiguration
}

protected createSpanInternal (name: string, options: ReactNativeSpanOptions, attributes: SpanAttributes) {
if (!NativeBugsnagPerformance || !this.attachedToNative || options.isFirstClass !== true || options.doNotDelegateToNativeSDK === true) {
if (!this.attachedToNative || options.isFirstClass !== true || options.doNotDelegateToNativeSDK === true) {
return super.createSpanInternal(name, options, attributes)
}

Expand All @@ -37,7 +37,7 @@ export class ReactNativeSpanFactory extends SpanFactory<ReactNativeConfiguration

protected discardSpan (span: NativeSpanInternal) {
if (span.isNativeSpan) {
NativeBugsnagPerformance?.discardNativeSpan(span.id, span.traceId)
NativeBugsnagPerformance.discardNativeSpan(span.id, span.traceId)
}

super.discardSpan(span)
Expand All @@ -55,9 +55,9 @@ export class ReactNativeSpanFactory extends SpanFactory<ReactNativeConfiguration
const unixEndTimeNanos = (this.clock as ReactNativeClock).toUnixNanoseconds(endTime)
const attributes = spanEnded.attributes.toObject()
delete attributes['bugsnag.sampling.p']
NativeBugsnagPerformance?.endNativeSpan(spanEnded.id, spanEnded.traceId, unixEndTimeNanos, attributes)
NativeBugsnagPerformance.endNativeSpan(spanEnded.id, spanEnded.traceId, unixEndTimeNanos, attributes)
} else {
NativeBugsnagPerformance?.discardNativeSpan(spanEnded.id, spanEnded.traceId)
NativeBugsnagPerformance.discardNativeSpan(spanEnded.id, spanEnded.traceId)
}
}

Expand Down
16 changes: 4 additions & 12 deletions packages/platforms/react-native/tests/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,24 +70,16 @@ beforeEach(() => {

describe('React Native client tests', () => {
describe('attach()', () => {
it('logs a warning and noops if native performance is not available', () => {
it('throws an error if native performance is not available', () => {
turboModule.isNativePerformanceAvailable = jest.fn().mockReturnValue(false)

client = require('../lib/client').default
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {})

client.attach({})
expect(warnSpy).toHaveBeenCalledWith('Could not attach to native SDK. No compatible version of Bugsnag Cocoa Performance was found.')
expect(client.attach).toThrow('Could not attach to native SDK. No compatible version of Bugsnag Cocoa Performance was found.')
})

it('logs a warning and noops if native performance has not been started', () => {
it('throws an error if native performance has not been started', () => {
turboModule.attachToNativeSDK = jest.fn().mockReturnValue(null)

client = require('../lib/client').default
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {})

client.attach()
expect(warnSpy).toHaveBeenCalledWith('Could not attach to native SDK. Bugsnag Cocoa Performance has not been started.')
expect(client.attach).toThrow('Could not attach to native SDK. Bugsnag Cocoa Performance has not been started.')
})

it('starts the client using the native configuration', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { ReactNativeSpanFactory } from '../lib/span-factory'
import createSchema from '../lib/config'
import { platformExtensions } from '../lib/platform-extensions'
import { createTestClient, IncrementingClock, InMemoryDelivery, VALID_API_KEY } from '@bugsnag/js-performance-test-utilities'

Expand All @@ -12,7 +11,7 @@ describe('startNavigationSpan', () => {
const testClient = createTestClient({
deliveryFactory: () => delivery,
spanFactory: ReactNativeSpanFactory,
platformExtensions: (spanFactory, spanContextStorage) => platformExtensions(0, clock, createSchema(), spanFactory as ReactNativeSpanFactory, spanContextStorage)
platformExtensions: (spanFactory, spanContextStorage) => platformExtensions(0, clock, spanFactory as ReactNativeSpanFactory, spanContextStorage)
})

testClient.start({ apiKey: VALID_API_KEY })
Expand Down
45 changes: 22 additions & 23 deletions packages/platforms/react-native/tests/span-factory.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @typescript-eslint/no-non-null-assertion */
import { ReactNativeSpanFactory } from '../lib/span-factory'
import NativeBugsnagPerformance from '../lib/native'
import { ControllableBackgroundingListener, InMemoryProcessor, spanAttributesSource, StableIdGenerator } from '@bugsnag/js-performance-test-utilities'
Expand Down Expand Up @@ -49,7 +48,7 @@ describe('ReactNativeSpanFactory', () => {

const startTime = clock.now()
const nativeSpan = spanFactory.startSpan('native span', { startTime, isFirstClass: true })
expect(NativeBugsnagPerformance!.startNativeSpan).toHaveBeenCalledWith('native span', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(NativeBugsnagPerformance.startNativeSpan).toHaveBeenCalledWith('native span', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(contextStorage.current).toBe(nativeSpan)
})

Expand All @@ -58,7 +57,7 @@ describe('ReactNativeSpanFactory', () => {

const startTime = clock.now()
const span = spanFactory.startSpan('not first class', { startTime, isFirstClass })
expect(NativeBugsnagPerformance!.startNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.startNativeSpan).not.toHaveBeenCalled()
expect(contextStorage.current).toBe(span)
})

Expand All @@ -67,14 +66,14 @@ describe('ReactNativeSpanFactory', () => {

const startTime = clock.now()
const span = spanFactory.startSpan('first class', { startTime, isFirstClass: true, doNotDelegateToNativeSDK: true })
expect(NativeBugsnagPerformance!.startNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.startNativeSpan).not.toHaveBeenCalled()
expect(contextStorage.current).toBe(span)
})

it('does not start a native span when not attached to native', () => {
const startTime = clock.now()
const span = spanFactory.startSpan('first class', { startTime, isFirstClass: true })
expect(NativeBugsnagPerformance!.startNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.startNativeSpan).not.toHaveBeenCalled()
expect(contextStorage.current).toBe(span)
})

Expand All @@ -84,7 +83,7 @@ describe('ReactNativeSpanFactory', () => {
const startTime = clock.now()
const parentContext = spanFactory.startSpan('parent', { startTime, isFirstClass: false })
const nativeSpan = spanFactory.startSpan('child', { startTime, isFirstClass: true, parentContext })
expect(NativeBugsnagPerformance!.startNativeSpan).toHaveBeenCalledWith('child', expect.objectContaining({ parentContext: { id: parentContext.id, traceId: parentContext.traceId } }))
expect(NativeBugsnagPerformance.startNativeSpan).toHaveBeenCalledWith('child', expect.objectContaining({ parentContext: { id: parentContext.id, traceId: parentContext.traceId } }))
expect(nativeSpan.id).toBe('native-span-id')
expect(nativeSpan.traceId).toBe(parentContext.traceId)
expect(nativeSpan.parentSpanId).toBe(parentContext.id)
Expand All @@ -96,7 +95,7 @@ describe('ReactNativeSpanFactory', () => {

const startTime = clock.now()
const nativeSpan = spanFactory.startSpan('child', { startTime, isFirstClass: true, parentContext })
expect(NativeBugsnagPerformance!.startNativeSpan).toHaveBeenCalledWith('child', expect.objectContaining({ parentContext: undefined }))
expect(NativeBugsnagPerformance.startNativeSpan).toHaveBeenCalledWith('child', expect.objectContaining({ parentContext: undefined }))
expect(nativeSpan.id).toBe('native-span-id')
expect(nativeSpan.traceId).toBe('native-trace-id')
expect(contextStorage.current).toBe(nativeSpan)
Expand All @@ -109,7 +108,7 @@ describe('ReactNativeSpanFactory', () => {

const startTime = clock.now()
const nativeSpan = spanFactory.startSpan('native span', { startTime, isFirstClass: true })
expect(NativeBugsnagPerformance!.startNativeSpan).toHaveBeenCalledWith('native span', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(NativeBugsnagPerformance.startNativeSpan).toHaveBeenCalledWith('native span', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(contextStorage.current).toBe(nativeSpan)

const endTime = clock.now()
Expand All @@ -118,7 +117,7 @@ describe('ReactNativeSpanFactory', () => {

expect(contextStorage.current).toBeUndefined()
expect(processor.spans.length).toBe(0)
expect(NativeBugsnagPerformance!.endNativeSpan).toHaveBeenCalledWith(
expect(NativeBugsnagPerformance.endNativeSpan).toHaveBeenCalledWith(
nativeSpan.id,
nativeSpan.traceId,
clock.toUnixNanoseconds(endTime),
Expand All @@ -129,14 +128,14 @@ describe('ReactNativeSpanFactory', () => {
spanFactory.onAttach()
const startTime = clock.now()
const span = spanFactory.startSpan('not native', { startTime, isFirstClass: false })
expect(NativeBugsnagPerformance!.startNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.startNativeSpan).not.toHaveBeenCalled()
expect(contextStorage.current).toBe(span)

const endTime = clock.now()
spanFactory.endSpan(span, endTime)
await jest.runOnlyPendingTimersAsync()

expect(NativeBugsnagPerformance!.endNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.endNativeSpan).not.toHaveBeenCalled()
expect(contextStorage.current).toBeUndefined()
expect(processor.spans.length).toBe(1)
})
Expand All @@ -151,27 +150,27 @@ describe('ReactNativeSpanFactory', () => {
spanFactory.configure({ logger: jestLogger, onSpanEnd: [onSpanEndCallback] } as unknown as InternalConfiguration<ReactNativeConfiguration>)
const startTime = clock.now()
const validSpan = spanFactory.startSpan('should send', { startTime, isFirstClass: true })
expect(NativeBugsnagPerformance!.startNativeSpan).toHaveBeenCalledWith('should send', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(NativeBugsnagPerformance.startNativeSpan).toHaveBeenCalledWith('should send', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(contextStorage.current).toBe(validSpan)

const invalidSpan = spanFactory.startSpan('should discard', { startTime, isFirstClass: true })
expect(NativeBugsnagPerformance!.startNativeSpan).toHaveBeenCalledWith('should discard', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(NativeBugsnagPerformance.startNativeSpan).toHaveBeenCalledWith('should discard', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(contextStorage.current).toBe(invalidSpan)

const endTime = clock.now()
spanFactory.endSpan(invalidSpan, endTime)
await jest.runOnlyPendingTimersAsync()

expect(onSpanEndCallback).toHaveBeenCalledTimes(1)
expect(NativeBugsnagPerformance!.endNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance!.discardNativeSpan).toHaveBeenCalledTimes(1)
expect(NativeBugsnagPerformance.endNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.discardNativeSpan).toHaveBeenCalledTimes(1)

spanFactory.endSpan(validSpan, endTime)
await jest.runOnlyPendingTimersAsync()

expect(onSpanEndCallback).toHaveBeenCalledTimes(2)
expect(NativeBugsnagPerformance!.endNativeSpan).toHaveBeenCalledTimes(1)
expect(NativeBugsnagPerformance!.discardNativeSpan).toHaveBeenCalledTimes(1)
expect(NativeBugsnagPerformance.endNativeSpan).toHaveBeenCalledTimes(1)
expect(NativeBugsnagPerformance.discardNativeSpan).toHaveBeenCalledTimes(1)

expect(processor.spans.length).toBe(0)
})
Expand All @@ -183,23 +182,23 @@ describe('ReactNativeSpanFactory', () => {

const startTime = clock.now()
const nativeSpan = spanFactory.startSpan('native span', { startTime, isFirstClass: true })
expect(NativeBugsnagPerformance!.startNativeSpan).toHaveBeenCalledWith('native span', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))
expect(NativeBugsnagPerformance.startNativeSpan).toHaveBeenCalledWith('native span', expect.objectContaining({ startTime: clock.toUnixNanoseconds(startTime) }))

spanFactory.endSpan(nativeSpan, DISCARD_END_TIME)
expect(NativeBugsnagPerformance!.endNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance!.discardNativeSpan).toHaveBeenCalledWith(nativeSpan.id, nativeSpan.traceId)
expect(NativeBugsnagPerformance.endNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.discardNativeSpan).toHaveBeenCalledWith(nativeSpan.id, nativeSpan.traceId)
})

it('does not call discardNativeSpan for non-native spans', () => {
spanFactory.onAttach()

const startTime = clock.now()
const span = spanFactory.startSpan('not native', { startTime, isFirstClass: false })
expect(NativeBugsnagPerformance!.startNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.startNativeSpan).not.toHaveBeenCalled()

spanFactory.endSpan(span, DISCARD_END_TIME)
expect(NativeBugsnagPerformance!.discardNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance!.endNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.discardNativeSpan).not.toHaveBeenCalled()
expect(NativeBugsnagPerformance.endNativeSpan).not.toHaveBeenCalled()
expect(processor.spans.length).toBe(0)
})
})
Expand Down

0 comments on commit 9c595f0

Please sign in to comment.