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

Conversation

gingerbenw
Copy link
Member

Goal

Facilitate trace correlation with the performance client

Design

Add new method to events to enable adding span and trace id's, which can be leveraged by bugsnag-js-performance to correlate traces and errors, but can also be called manually if required

Changeset

Add a new setTraceCorrelation method to events

Testing

Add unit test that calls setTraceCorrelation and verifies the span and trace id are present in the serialised event

@gingerbenw gingerbenw force-pushed the PLAT-12224/event-correlation branch from d23564f to e49ba2a Compare June 14, 2024 08:46
Copy link

github-actions bot commented Jun 14, 2024

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 43.80 kB 13.43 kB
After 44.22 kB 13.52 kB
± ⚠️ +420 bytes ⚠️ +88 bytes

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against 91611eb

@gingerbenw gingerbenw requested a review from imjoehaines June 17, 2024 11:31
@@ -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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants