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

feat: Use new AM SDK API #19041

Merged
merged 2 commits into from
May 28, 2020
Merged

feat: Use new AM SDK API #19041

merged 2 commits into from
May 28, 2020

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented May 27, 2020

This bumps the JS SDK to the new beta using the new AM API. getsentry/sentry-javascript#2600

@HazAT HazAT requested review from billyvg and dashed May 27, 2020 14:30
@HazAT HazAT self-assigned this May 27, 2020
span.finish();
span.timestamp = start + sizeInKb / 1000;
transaction.finish(true);
span.finish(start + sizeInKb / 1000);
Copy link
Member Author

@HazAT HazAT May 27, 2020

Choose a reason for hiding this comment

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

@billyvg I made the changes to the file to use the new API.
But this line here in terms of calculating the endTimestamp for the Span seems very odd.
Time + Filesize? What is the reason behind this?
I suppose you are abusing the Span chart to visualize file size?

Copy link
Member

Choose a reason for hiding this comment

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

Correct

Comment on lines +89 to +92
});

Sentry.addGlobalEventProcessor(async event => {
return normalizeTransactionName(appRoutes, event);
Copy link
Member Author

@HazAT HazAT May 27, 2020

Choose a reason for hiding this comment

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

@dashed Transaction still goes through event processors and this is the internal workaround for it.
EventProcessors are considered internal. And they work the same as beforeSend :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh Interesting 👍

Copy link
Member

@dashed dashed left a comment

Choose a reason for hiding this comment

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

Changes for src/sentry/static/sentry/app/bootstrap.tsx looks good to me 👍

@HazAT
Copy link
Member Author

HazAT commented May 27, 2020

@dashed Are the percy diffs flakey as always wrt to the performance page?

@dashed
Copy link
Member

dashed commented May 27, 2020

@HazAT ah that might be something we'll have to address. 👍 I approved the Percy snapshots.

@HazAT
Copy link
Member Author

HazAT commented May 27, 2020

Will merge this tomorrow morning

@HazAT HazAT merged commit ffb8518 into master May 28, 2020
@HazAT HazAT deleted the hazat/js-beta branch May 28, 2020 05:50
HazAT added a commit that referenced this pull request May 28, 2020
HazAT added a commit that referenced this pull request May 28, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants