Merge pull request #559 from bugsnag/PLAT-13377/navigation-spans-rend…

…ering-metrics

Do not delegate automatic navigation spans to native SDK

packages/core/lib/span-factory.ts

@@ -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'
@@ -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
 
@@ -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) {
@@ -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) {

packages/platforms/react-native/lib/index.ts

@@ -5,5 +5,4 @@ export type { PlatformExtensions } from './platform-extensions'
 
 export default BugsnagPerformance
 
-export { createNavigationSpan } from './create-navigation-span'
 export * from './span-factory'

packages/platforms/react-native/lib/platform-extensions.tsx

@@ -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) {

packages/platforms/react-native/lib/span-factory.ts

@@ -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'
@@ -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) {
@@ -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
+  }
 }

packages/platforms/react-native/tests/platform-extensions.test.ts

@@ -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'
@@ -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 })

packages/platforms/react-native/tests/span-factory.test.ts

@@ -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 })
@@ -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' } })
+    })
+  })
 })

packages/plugin-react-native-navigation/__mocks__/@bugsnag/react-native-performance.ts

@@ -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>> = []
 

packages/plugin-react-native-navigation/__tests__/react-native-navigation-plugin.test.ts

@@ -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'
@@ -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
@@ -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
@@ -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

packages/plugin-react-native-navigation/lib/react-native-navigation-plugin.ts

@@ -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
@@ -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) {

packages/plugin-react-navigation/__mocks__/@bugsnag/react-native-performance.ts

@@ -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>> = []
 

packages/plugin-react-navigation/lib/navigation-context.tsx

@@ -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: () => {},
@@ -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) {