Skip to content

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

Closed
wants to merge 4 commits into from

Conversation

dashed
Copy link
Member

@dashed dashed commented May 19, 2020

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.

@dashed dashed requested a review from rhcarvalho May 19, 2020 16:27
@dashed dashed requested a review from kamilogorek as a code owner May 19, 2020 16:27
@dashed dashed self-assigned this May 19, 2020
@getsentry-bot
Copy link
Contributor

getsentry-bot commented May 19, 2020

Warnings
⚠️ Please add a changelog entry for your changes.
Messages
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 16.9277 kB) (ES6: 15.959 kB)

📖 ✅ TSLint passed

Generated by 🚫 dangerJS against cb005be

} catch (_oO) {
// We are dealing with an envelope here
// For simplicity we only deal with transactions
const envelopeLines = serializedData.split('\n');
Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, per envelope spec.

Copy link
Member Author

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);
Copy link
Contributor

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?

@HazAT HazAT closed this in #2615 May 27, 2020
@HazAT HazAT deleted the add-breadcrumb-envelope-test branch July 23, 2020 11:08
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.

5 participants