Skip to content

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.

6 participants