-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
test(breadcrumb): Add test case for creating breadcrumbs for requests to the envelope endpoint #2603
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
Conversation
} catch (_oO) { | ||
// We are dealing with an envelope here | ||
// For simplicity we only deal with transactions | ||
const envelopeLines = serializedData.split('\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HazAT Is it guaranteed that the none of the entries will contain newlines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, per envelope spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HazAT
I read over the spec. There's a chance the item payload could contain newlines; and this could fail:
const item = JSON.parse(envelopeLines[2]);
Maybe something like this?
const [envelopeHeaderRaw, itemHeaderRaw, ...rest] = serializedData.split('\n');
const envelopeHeader = JSON.parse(envelopeHeaderRaw);
const itemHeader = JSON.parse(itemHeaderRaw);
const body = rest.join('\n');
} catch (_oO) { | ||
logger.error('Error while adding sentry type breadcrumb'); | ||
} catch (error) { | ||
logger.error('Error while adding sentry type breadcrumb will try envelope', error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will add a lot of noise when we know envelopes are not "exceptional behavior".
What if we explicitly selected whether to try to decode application/x-sentry-envelope
vs application/json
, and then log a potential error as before?
There must be a way to distinguish the two without parsing all input. Or maybe a way for addSentryBreadcrumb
to get the event object directly instead of a serialized version of it.
As it is implemented now, this error path requires deserializing the event and maybe that's not necessary?
This adds a test case for creating breadcrumbs whenever an event is sent to the envelope endpoint.
This fails for now until #2602 is resolved.