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

[PLAT-12224] Add setTraceCorrelation method to event #2159

Merged
merged 3 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## [Unreleased]

### Added

- (core) Add new `setTraceCorrelation` method to events [#2159](https://github.com/bugsnag/bugsnag-js/pull/2159)

### Fixed

- (react-native) Use synchronous native module calls when New Architecture is enabled [#2152](https://github.com/bugsnag/bugsnag-js/pull/2152)
Expand Down
2 changes: 2 additions & 0 deletions packages/core/event.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export default class EventWithInternals extends Event {
_featuresIndex: { [key: string]: number }
_user: User
_handledState: HandledState
_correlation?: { spanId: string, traceId: string }
_session?: Session
toJSON(): {
payloadVersion: '4'
Expand All @@ -40,6 +41,7 @@ export default class EventWithInternals extends Event {
request: Request
breadcrumbs: Breadcrumb[]
context: string | undefined
correlation: { spanId: string, traceId: string } | undefined
groupingHash: string | undefined
metaData: { [key: string]: any }
user: User
Expand Down
17 changes: 16 additions & 1 deletion packages/core/event.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class Event {
this._featuresIndex = {}
this._user = {}
this._session = undefined
this._correlation = undefined

this.errors = [
createBugsnagError(errorClass, errorMessage, Event.__type, stacktrace)
Expand All @@ -48,6 +49,19 @@ class Event {
return metadataDelegate.add(this._metadata, section, keyOrObj, maybeVal)
}

/**
* Associate this event with a specific trace. This is usually done automatically when
* using bugsnag-js-performance, but can also be set manually if required.
*
* @param traceId the ID of the trace the event occurred within
* @param spanId the ID of the span that the event occurred within
*/
setTraceCorrelation (traceId, spanId) {
if (traceId != null) {
this._correlation = { traceId, spanId }
}
}

getMetadata (section, key) {
return metadataDelegate.get(this._metadata, section, key)
}
Expand Down Expand Up @@ -101,7 +115,8 @@ class Event {
metaData: this._metadata,
user: this._user,
session: this._session,
featureFlags: this.getFeatureFlags()
featureFlags: this.getFeatureFlags(),
correlation: this._correlation
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions packages/core/test/event.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -420,4 +420,13 @@ describe('@bugsnag/core/event', () => {
}))
})
})

describe('event.setTraceCorrelation()', () => {
it('allows setting the span and trace id', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a couple of scenarios missing tests:

  • if spanId is undefined or null it's still added
  • if traceId is undefined or null it's not added

Copy link
Member Author

@gingerbenw gingerbenw Jun 19, 2024

Choose a reason for hiding this comment

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

Should we also update the api for setTraceCorrelation to accept undefined | null for the spanId?

public setTraceCorrelation(traceId: string, spanId?: string | null): void

Copy link
Member Author

Choose a reason for hiding this comment

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

could they both be expected to be null? 🤔

Copy link
Contributor

@imjoehaines imjoehaines Jun 19, 2024

Choose a reason for hiding this comment

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

could they both be expected to be null? 🤔

Presumably we shouldn't call setTraceCorrelation if there's no trace ID so I think only span ID should be optional in the API

I don't think it makes sense for span ID to be null as an expected value (i.e. in the TS type definition) but it's javascript so it can easily end up that way at runtime

so I think the signature should be:

setTraceCorrelation(traceId: string, spanId?: string): void

but in the method we can't assume they have the right types

const event = new Event('Err', 'bad', [])
event.setTraceCorrelation('test-trace-id', 'test-span-id')
const serialized = event.toJSON()
expect(serialized.correlation).toEqual({ traceId: 'test-trace-id', spanId: 'test-span-id' })
})
})
})
3 changes: 3 additions & 0 deletions packages/core/types/event.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ declare class Event {
public addFeatureFlags(featureFlags: FeatureFlag[]): void
public clearFeatureFlag(name: string): void
public clearFeatureFlags(): void

// trace correlation
public setTraceCorrelation(traceId: string, spanId: string): void
}

interface HandledState {
Expand Down
Loading