Skip to content
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

Do not delegate automatic navigation spans to native SDK #559

Merged
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
19 changes: 8 additions & 11 deletions packages/core/lib/span-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type { IdGenerator } from './id-generator'
import type { NetworkSpanOptions } from './network-span'
import type { BufferingProcessor, Processor } from './processor'
import type { ReadonlySampler } from './sampler'
import type { InternalSpanOptions, ParentContext, Span, SpanOptionSchema, SpanOptions } from './span'
import type { InternalSpanOptions, Span, SpanOptionSchema, SpanOptions } from './span'
import { SpanInternal, coreSpanOptionSchema } from './span'
import type { SpanContextStorage } from './span-context'
import { timeToNumber } from './time'
Expand Down Expand Up @@ -64,12 +64,10 @@ export class SpanFactory<C extends Configuration> {
}

startSpan (name: string, options: SpanOptions) {
const safeStartTime = timeToNumber(this.clock, options.startTime)

// if the parentContext option is not set use the current context
// if parentContext is explicitly null, or there is no current context,
// we are starting a new root span
const parentContext = isParentContext(options.parentContext) || options.parentContext === null
options.parentContext = isParentContext(options.parentContext) || options.parentContext === null
? options.parentContext
: this.spanContextStorage.current

Expand All @@ -78,7 +76,7 @@ export class SpanFactory<C extends Configuration> {
attributes.set('bugsnag.span.first_class', options.isFirstClass)
}

const span = this.createSpanInternal(name, safeStartTime, parentContext, options.isFirstClass, attributes)
const span = this.createSpanInternal(name, options, attributes)

// don't track spans that are started while the app is backgrounded
if (this.isInForeground) {
Expand All @@ -94,15 +92,14 @@ export class SpanFactory<C extends Configuration> {

protected createSpanInternal (
name: string,
startTime: number,
parentContext: ParentContext | null | undefined,
isFirstClass: boolean | undefined,
options: SpanOptions,
attributes: SpanAttributes) {
const safeStartTime = timeToNumber(this.clock, options.startTime)
const spanId = this.idGenerator.generate(64)
const parentSpanId = parentContext ? parentContext.id : undefined
const traceId = parentContext ? parentContext.traceId : this.idGenerator.generate(128)
const parentSpanId = options.parentContext ? options.parentContext.id : undefined
const traceId = options.parentContext ? options.parentContext.traceId : this.idGenerator.generate(128)

return new SpanInternal(spanId, traceId, name, startTime, attributes, this.clock, parentSpanId)
return new SpanInternal(spanId, traceId, name, safeStartTime, attributes, this.clock, parentSpanId)
}

startNetworkSpan (options: NetworkSpanOptions) {
Expand Down
16 changes: 0 additions & 16 deletions packages/platforms/react-native/lib/create-navigation-span.ts

This file was deleted.

1 change: 0 additions & 1 deletion packages/platforms/react-native/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,4 @@ export type { PlatformExtensions } from './platform-extensions'

export default BugsnagPerformance

export { createNavigationSpan } from './create-navigation-span'
export * from './span-factory'
4 changes: 1 addition & 3 deletions packages/platforms/react-native/lib/platform-extensions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,14 @@ import { Platform } from 'react-native'
import NativeBugsnagPerformance from './native'
import type { ReactNativeAttachConfiguration, ReactNativeConfiguration, ReactNativeSchema } from './config'
import { createAppStartSpan } from './create-app-start-span'
import { createNavigationSpan } from './create-navigation-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) => ({
startNavigationSpan: function (routeName: string, spanOptions?: NavigationSpanOptions) {
const cleanOptions = spanFactory.validateSpanOptions(routeName, spanOptions)
cleanOptions.options.isFirstClass = true
const span = createNavigationSpan(spanFactory, cleanOptions.name, cleanOptions.options)
const span = spanFactory.startNavigationSpan(cleanOptions.name, cleanOptions.options)
return spanFactory.toPublicApi(span)
},
withInstrumentedAppStarts: function (App: React.FC) {
Expand Down
39 changes: 31 additions & 8 deletions packages/platforms/react-native/lib/span-factory.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { runSpanEndCallbacks, SpanFactory, SpanInternal } from '@bugsnag/core-performance'
import type { SpanAttributes, ParentContext } from '@bugsnag/core-performance'
import { runSpanEndCallbacks, SpanFactory, SpanInternal, timeToNumber } from '@bugsnag/core-performance'
import type { SpanAttributes, SpanOptions } from '@bugsnag/core-performance'
import type { ReactNativeConfiguration } from './config'
import NativeBugsnagPerformance from './native'
import type { ReactNativeClock } from './clock'
Expand All @@ -8,22 +8,31 @@ class NativeSpanInternal extends SpanInternal {
public readonly isNativeSpan: boolean = true
}

interface ReactNativeSpanOptions extends SpanOptions {
doNotDelegateToNativeSDK?: boolean
}

export class ReactNativeSpanFactory extends SpanFactory<ReactNativeConfiguration> {
private attachedToNative = false

onAttach () {
this.attachedToNative = true
}

protected createSpanInternal (name: string, startTime: number, parentContext: ParentContext | null | undefined, isFirstClass: boolean | undefined, attributes: SpanAttributes) {
if (!NativeBugsnagPerformance || !this.attachedToNative || isFirstClass !== true) {
return super.createSpanInternal(name, startTime, parentContext, isFirstClass, attributes)
startSpan (name: string, options: ReactNativeSpanOptions) {
return super.startSpan(name, options)
}

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

const unixStartTimeNanos = (this.clock as ReactNativeClock).toUnixNanoseconds(startTime)
const nativeParentContext = parentContext ? { id: parentContext.id, traceId: parentContext.traceId } : undefined
const safeStartTime = timeToNumber(this.clock, options.startTime)
const unixStartTimeNanos = (this.clock as ReactNativeClock).toUnixNanoseconds(safeStartTime)
const nativeParentContext = options.parentContext ? { id: options.parentContext.id, traceId: options.parentContext.traceId } : undefined
const nativeSpan = NativeBugsnagPerformance.startNativeSpan(name, { startTime: unixStartTimeNanos, parentContext: nativeParentContext })
return new NativeSpanInternal(nativeSpan.id, nativeSpan.traceId, name, startTime, attributes, this.clock, nativeSpan.parentSpanId)
return new NativeSpanInternal(nativeSpan.id, nativeSpan.traceId, name, safeStartTime, attributes, this.clock, nativeSpan.parentSpanId)
}

protected discardSpan (span: NativeSpanInternal) {
Expand Down Expand Up @@ -51,4 +60,18 @@ export class ReactNativeSpanFactory extends SpanFactory<ReactNativeConfiguration
NativeBugsnagPerformance?.discardNativeSpan(spanEnded.id, spanEnded.traceId)
}
}

startNavigationSpan (routeName: string, spanOptions: ReactNativeSpanOptions) {
// Navigation spans are always first class
spanOptions.isFirstClass = true

const spanName = '[Navigation]' + routeName
const span = this.startSpan(spanName, spanOptions)

// Default navigation span attributes
span.setAttribute('bugsnag.span.category', 'navigation')
span.setAttribute('bugsnag.navigation.route', routeName)

return span
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { ReactNativeSpanFactory } from '../lib'
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 @@ -11,7 +11,8 @@ describe('startNavigationSpan', () => {
const clock = new IncrementingClock()
const testClient = createTestClient({
deliveryFactory: () => delivery,
platformExtensions: (spanFactory, spanContextStorage) => platformExtensions(0, clock, createSchema(), spanFactory as unknown as ReactNativeSpanFactory, spanContextStorage)
spanFactory: ReactNativeSpanFactory,
platformExtensions: (spanFactory, spanContextStorage) => platformExtensions(0, clock, createSchema(), spanFactory as ReactNativeSpanFactory, spanContextStorage)
})

testClient.start({ apiKey: VALID_API_KEY })
Expand Down
39 changes: 39 additions & 0 deletions packages/platforms/react-native/tests/span-factory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,15 @@ describe('ReactNativeSpanFactory', () => {
expect(contextStorage.current).toBe(span)
})

it('does not start a native span when doNotDelegateToNativeSDK is true', () => {
spanFactory.onAttach()

const startTime = clock.now()
const span = spanFactory.startSpan('first class', { startTime, isFirstClass: true, doNotDelegateToNativeSDK: true })
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 })
Expand Down Expand Up @@ -194,4 +203,34 @@ describe('ReactNativeSpanFactory', () => {
expect(processor.spans.length).toBe(0)
})
})

describe('startNavigationSpan', () => {
it('sets the span name to the route prefixed with [Navigation]', () => {
const span = spanFactory.startNavigationSpan('testRoute', {})
const endedSpan = span.end(12345, spanFactory.sampler.spanProbability)

expect(endedSpan.name).toBe('[Navigation]testRoute')
})

it('always sets the span as first class', () => {
const span = spanFactory.startNavigationSpan('testRoute', { isFirstClass: false })
const endedSpan = span.end(12345, spanFactory.sampler.spanProbability)

expect(endedSpan.attributes.toJson()).toContainEqual({ key: 'bugsnag.span.first_class', value: { boolValue: true } })
})

it('includes navigation category attribute', () => {
const span = spanFactory.startNavigationSpan('testRoute', {})
const endedSpan = span.end(12345, spanFactory.sampler.spanProbability)

expect(endedSpan.attributes.toJson()).toContainEqual({ key: 'bugsnag.span.category', value: { stringValue: 'navigation' } })
})

it('includes the route attribute', () => {
const span = spanFactory.startNavigationSpan('testRoute', {})
const endedSpan = span.end(12345, spanFactory.sampler.spanProbability)

expect(endedSpan.attributes.toJson()).toContainEqual({ key: 'bugsnag.navigation.route', value: { stringValue: 'testRoute' } })
})
})
})
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import type { Configuration, Plugin } from '@bugsnag/core-performance'
import type { ReactNativeConfiguration } from '@bugsnag/react-native-performance'
export { createNavigationSpan } from '@bugsnag/react-native-performance/lib/create-navigation-span'

const plugins: Array<Plugin<Configuration>> = []

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { MockSpanFactory, createConfiguration } from '@bugsnag/js-performance-test-utilities'
import { MockReactNativeSpanFactory, createConfiguration } from '@bugsnag/js-performance-test-utilities'
import type { ReactNativeConfiguration } from '@bugsnag/react-native-performance'
import { Navigation } from 'react-native-navigation'
import ReactNativeNavigationPlugin from '../lib/react-native-navigation-plugin'
Expand All @@ -17,7 +17,7 @@ describe('ReactNativeNavigationPlugin', () => {
it('creates a navigation span when the route changes', () => {
const plugin = new ReactNativeNavigationPlugin(Navigation)
const configuration = createConfiguration<ReactNativeConfiguration>()
const spanFactory = new MockSpanFactory()
const spanFactory = new MockReactNativeSpanFactory()
plugin.configure(configuration, spanFactory)

// Simulate a route change
Expand Down Expand Up @@ -49,7 +49,7 @@ describe('ReactNativeNavigationPlugin', () => {
it('does not end the current navigation while there are components waiting', () => {
const plugin = new ReactNativeNavigationPlugin(Navigation)
const configuration = createConfiguration<ReactNativeConfiguration>()
const spanFactory = new MockSpanFactory()
const spanFactory = new MockReactNativeSpanFactory()
plugin.configure(configuration, spanFactory)

// Simulate a route change
Expand Down Expand Up @@ -98,7 +98,7 @@ describe('ReactNativeNavigationPlugin', () => {
it('discards the active navigation span when the route changes', () => {
const plugin = new ReactNativeNavigationPlugin(Navigation)
const configuration = createConfiguration<ReactNativeConfiguration>()
const spanFactory = new MockSpanFactory()
const spanFactory = new MockReactNativeSpanFactory()
plugin.configure(configuration, spanFactory)

// Simulate a route change
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import type { Plugin, SpanFactory, SpanInternal } from '@bugsnag/core-performanc
import type { ReactNativeConfiguration, ReactNativeSpanFactory } from '@bugsnag/react-native-performance'
import type { NavigationDelegate } from 'react-native-navigation/lib/dist/src/NavigationDelegate'

import { createNavigationSpan } from '@bugsnag/react-native-performance'

type Reason = 'immediate' | 'mount' | 'unmount' | 'condition'

const NAVIGATION_START_TIMEOUT = 1000
Expand Down Expand Up @@ -80,11 +78,11 @@ class BugsnagPluginReactNativeNavigationPerformance implements Plugin<ReactNativ

// Navigation has occurred
this.Navigation.events().registerComponentWillAppearListener(event => {
if (typeof this.startTime === 'number') {
if (this.spanFactory && typeof this.startTime === 'number') {
clearTimeout(this.startTimeout)

const routeName = event.componentName
this.currentNavigationSpan = createNavigationSpan(spanFactory as ReactNativeSpanFactory, routeName, { startTime: this.startTime })
this.currentNavigationSpan = this.spanFactory.startNavigationSpan(routeName, { startTime: this.startTime, doNotDelegateToNativeSDK: true })
this.currentNavigationSpan.setAttribute('bugsnag.navigation.triggered_by', '@bugsnag/plugin-react-native-navigation-performance')

if (this.previousRoute) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import type { Configuration, Plugin } from '@bugsnag/core-performance'
import type { ReactNativeConfiguration } from '@bugsnag/react-native-performance'
export { createNavigationSpan } from '@bugsnag/react-native-performance/lib/create-navigation-span'

const plugins: Array<Plugin<Configuration>> = []

Expand Down
3 changes: 1 addition & 2 deletions packages/plugin-react-navigation/lib/navigation-context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import type { ReactNativeSpanFactory } from '@bugsnag/react-native-performance'
import type { PropsWithChildren } from 'react'

import React from 'react'
import { createNavigationSpan } from '@bugsnag/react-native-performance'

export const NavigationContext = React.createContext({
blockNavigationEnd: () => {},
Expand Down Expand Up @@ -73,7 +72,7 @@ export class NavigationContextProvider extends React.Component<Props> {
spanFactory.endSpan(this.currentSpan, DISCARDED)
}

const span = createNavigationSpan(spanFactory, currentRoute, { startTime: updateTime })
const span = spanFactory.startNavigationSpan(currentRoute, { startTime: updateTime, doNotDelegateToNativeSDK: true })
span.setAttribute('bugsnag.navigation.triggered_by', '@bugsnag/plugin-react-navigation-performance')

if (this.previousRoute) {
Expand Down
Loading
Loading