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

Update current state in page load and navigation plugins #553

Open
wants to merge 20 commits into
base: integration/component-rendering
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 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
6 changes: 3 additions & 3 deletions packages/core/lib/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export interface ClientOptions<S extends CoreSchema, C extends Configuration, T>
resourceAttributesSource: ResourceAttributeSource<C>
spanAttributesSource: SpanAttributesSource<C>
schema: S
plugins: (spanFactory: SpanFactory<C>, spanContextStorage: SpanContextStorage) => Array<Plugin<C>>
plugins: (spanFactory: SpanFactory<C>, spanContextStorage: SpanContextStorage, setAppState: (appState: AppState) => void, appState: AppState) => Array<Plugin<C>>
persistence: Persistence
retryQueueFactory: RetryQueueFactory
spanContextStorage?: SpanContextStorage
Expand Down Expand Up @@ -74,7 +74,7 @@ export function createClient<S extends CoreSchema, C extends Configuration, T> (
const setAppState = (state: AppState) => {
appState = state
}
const plugins = options.plugins(spanFactory, spanContextStorage)
const plugins = options.plugins(spanFactory, spanContextStorage, setAppState, appState)

return {
start: (config: C | string) => {
Expand Down Expand Up @@ -149,7 +149,7 @@ export function createClient<S extends CoreSchema, C extends Configuration, T> (
}

for (const plugin of plugins) {
plugin.configure(configuration, spanFactory, setAppState)
plugin.configure(configuration, spanFactory, setAppState, appState)
}
},
startSpan: (name, spanOptions?: SpanOptions) => {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/lib/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ import type { AppState } from './core'
import type { SpanFactory } from './span-factory'

export interface Plugin<C extends Configuration> {
configure: (configuration: InternalConfiguration<C>, spanFactory: SpanFactory<C>, setAppState: (state: AppState) => void) => void
configure: (configuration: InternalConfiguration<C>, spanFactory: SpanFactory<C>, setAppState: (state: AppState) => void, state?: AppState) => void
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getPermittedAttributes } from '../send-page-attributes'
import type { WebVitals } from '../web-vitals'
import { instrumentPageLoadPhaseSpans } from './page-load-phase-spans'
import { defaultRouteResolver } from '../default-routing-provider'
import type { AppState } from '../../../../core/lib/core'

export class FullPageLoadPlugin implements Plugin<BrowserConfiguration> {
private readonly spanFactory: SpanFactory<BrowserConfiguration>
Expand All @@ -14,6 +15,8 @@ export class FullPageLoadPlugin implements Plugin<BrowserConfiguration> {
private readonly onSettle: OnSettle
private readonly webVitals: WebVitals
private readonly performance: PerformanceWithTiming
private readonly setAppState: (appState: AppState) => void
private readonly appState: AppState

// if the page was backgrounded at any point in the loading process a page
// load span is invalidated as the browser will deprioritise the page
Expand All @@ -26,14 +29,18 @@ export class FullPageLoadPlugin implements Plugin<BrowserConfiguration> {
webVitals: WebVitals,
onSettle: OnSettle,
backgroundingListener: BackgroundingListener,
performance: PerformanceWithTiming
performance: PerformanceWithTiming,
setAppState: (appState: AppState) => void,
appState: AppState
) {
this.document = document
this.location = location
this.spanFactory = spanFactory
this.webVitals = webVitals
this.onSettle = onSettle
this.performance = performance
this.setAppState = setAppState
this.appState = appState

backgroundingListener.onStateChange(state => {
if (!this.wasBackgrounded && state === 'in-background') {
Expand Down Expand Up @@ -87,6 +94,9 @@ export class FullPageLoadPlugin implements Plugin<BrowserConfiguration> {

this.webVitals.attachTo(span)
this.spanFactory.endSpan(span, endTime)
if (this.appState === 'starting') {
this.setAppState('ready')
}
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { BrowserConfiguration } from '../config'
import type { RouteChangeSpanEndOptions, RouteChangeSpanOptions } from '../routing-provider'
import { getPermittedAttributes } from '../send-page-attributes'
import { defaultRouteResolver } from '../default-routing-provider'
import type { AppState } from '../../../../core/lib/core'
Copy link
Member

@gingerbenw gingerbenw Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an import from @bugsnag/core-performance and not a relative import

Copy link
Author

@AnastasiiaSvietlova AnastasiiaSvietlova Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this import needs to be fixed in all plugins?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, if we need to use code from another library, it should be exported and imported appropriately.


// exclude isFirstClass from the route change option schema
const { startTime, parentContext, makeCurrentContext } = coreSpanOptionSchema
Expand All @@ -27,7 +28,8 @@ export class RouteChangePlugin implements Plugin<BrowserConfiguration> {
constructor (
private readonly spanFactory: SpanFactory<BrowserConfiguration>,
private readonly location: Location,
private readonly document: Document
private readonly document: Document,
private readonly setAppState: (appState: AppState) => void
) {}

configure (configuration: InternalConfiguration<BrowserConfiguration>) {
Expand Down Expand Up @@ -61,7 +63,7 @@ export class RouteChangePlugin implements Plugin<BrowserConfiguration> {
} satisfies Span
}
}

this.setAppState('navigating')
// create internal options for validation
const routeChangeSpanOptions = {
...options,
Expand All @@ -79,7 +81,6 @@ export class RouteChangePlugin implements Plugin<BrowserConfiguration> {
// update the span name using the validated route
cleanOptions.name += route
const span = this.spanFactory.startSpan(cleanOptions.name, cleanOptions.options)

span.setAttribute('bugsnag.span.category', 'route_change')
span.setAttribute('bugsnag.browser.page.route', route)
span.setAttribute('bugsnag.browser.page.previous_route', previousRoute)
Expand Down Expand Up @@ -125,6 +126,7 @@ export class RouteChangePlugin implements Plugin<BrowserConfiguration> {
}

this.spanFactory.toPublicApi(span).end(options.endTime)
this.setAppState('ready')
}

} satisfies Span
Expand Down
8 changes: 5 additions & 3 deletions packages/platforms/browser/lib/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ if (typeof window === 'undefined' || typeof document === 'undefined') {
deliveryFactory: createFetchDeliveryFactory(window.fetch, clock, backgroundingListener),
idGenerator,
schema: createSchema(window.location.hostname, new DefaultRoutingProvider()),
plugins: (spanFactory, spanContextStorage) => [
plugins: (spanFactory, spanContextStorage, setAppState, appState) => [
onSettle,
new FullPageLoadPlugin(
document,
Expand All @@ -59,13 +59,15 @@ if (typeof window === 'undefined' || typeof document === 'undefined') {
webVitals,
onSettle,
backgroundingListener,
performance
performance,
setAppState,
appState
),
// ResourceLoadPlugin should always come after FullPageLoad plugin, as it should use that
// span context as the parent of it's spans
new ResourceLoadPlugin(spanFactory, spanContextStorage, window.PerformanceObserver),
new NetworkRequestPlugin(spanFactory, spanContextStorage, fetchRequestTracker, xhrRequestTracker),
new RouteChangePlugin(spanFactory, window.location, document)
new RouteChangePlugin(spanFactory, window.location, document, setAppState)
],
persistence,
retryQueueFactory: (delivery, retryQueueMaxSize) => new InMemoryQueue(delivery, retryQueueMaxSize)
Expand Down
Loading
Loading