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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: wrap method, profiler with App start and Stall tracking #1728

Merged
merged 14 commits into from
Aug 29, 2021

Conversation

jennmueng
Copy link
Member

@jennmueng jennmueng commented Aug 14, 2021

馃摙 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

馃摐 Description

Adds a new Sentry.wrap method that wraps the app with a React Profiler to profile the app start, and a touch event boundary. Currently, we wrap the root component with just a ReactNativeProfiler and a TouchEventBoundary. We will be able to instrument more things in the future (such as possibly auto-detect routing instrumentation, transactions triggered by touch events, etc).

Now, the app start finishes when the root component finishes mounting. If Sentry.wrap isn't used, then the app start will fall back to the Javascript global scope initialization timestamp.

Screen Shot 2021-08-24 at 11 29 31 AM

Other Changes

  • Bumps sentry-javascript dependencies to 6.12.0-beta.2.
  • Refactors the stall tracking integration to now be part of the ReactNativeTracing package for consistency.
  • Condenses the option flags for auto performance into enableAutoPerformanceTracking, which will be enabled by default. This means that you will need to just provide a sample rate to start using auto performance.
  • Individual enableAppStartTracking, enableNativeFramesTracking, and enableStallTracking flags available as options to ReactNativeTracing.

馃挕 Motivation and Context

Part of #1701

馃挌 How did you test it?

Wrote, updated unit tests and tested on both iOS and Android sample apps.

馃摑 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

@jennmueng jennmueng changed the title Jenn/init with feat: Add initWith method which wraps the root component and allows further instrumentation out of the box Aug 14, 2021
@jennmueng jennmueng changed the title feat: Add initWith method which wraps the root component and allows further instrumentation out of the box feat: Add initWith method to wrap the root component and allows further instrumentation ootb Aug 14, 2021
@jennmueng jennmueng changed the base branch from master to jenn/frames August 14, 2021 09:04
Base automatically changed from jenn/frames to master August 25, 2021 15:34
@bruno-garcia bruno-garcia mentioned this pull request Aug 25, 2021
@bruno-garcia bruno-garcia added this to Review in progress in kanban Aug 25, 2021
@bruno-garcia bruno-garcia moved this from Review in progress to In progress in kanban Aug 25, 2021
@jennmueng jennmueng changed the title feat: Add initWith method to wrap the root component and allows further instrumentation ootb feat: Add initWith method, RN Profiler for better App start, and move Stall Tracking into ReactNativeTracing Aug 26, 2021
@jennmueng jennmueng marked this pull request as ready for review August 26, 2021 22:05
@github-actions
Copy link
Contributor

github-actions bot commented Aug 26, 2021

Fails
馃毇 Please consider adding a changelog entry for the next release.
Messages
馃摉 Do not forget to update Sentry-docs with your feature once the pull request gets approved.
Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section under the following heading:

  1. feat: For new user-visible functionality.
  2. fix: For user-visible bug fixes.
  3. ref: For features, refactors and bug fixes in internal operations.

To the changelog entry, please add a link to this PR (consider a more descriptive message):

- wrap method, profiler with App start and Stall tracking. (1728)

If none of the above apply, you can opt out by adding #skip-changelog to the PR description.

Generated by 馃毇 dangerJS against 270f30c

"@sentry/tracing": "6.7.1",
"@sentry/types": "6.7.1",
"@sentry/utils": "6.7.1",
"@sentry/browser": "6.12.0-beta.2",
Copy link
Member

Choose a reason for hiding this comment

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

Let鈥檚 not bump to the beta please.

Copy link
Member

Choose a reason for hiding this comment

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

We won't if you release it before Tuesday next week :)

Copy link
Member

Choose a reason for hiding this comment

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

We'll make it happen :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!
We can merge this and ship another beta still today if there's time.

Next week bump deps to GA and GA this too

@AbhiPrasad
Copy link
Member

Can we not do the App wrapping after we init the sdk? This way we can catch errors that occur before we wrap the app?

"@sentry/tracing": "6.7.1",
"@sentry/types": "6.7.1",
"@sentry/utils": "6.7.1",
"@sentry/browser": "6.12.0-beta.2",
Copy link
Member

Choose a reason for hiding this comment

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

We won't if you release it before Tuesday next week :)

kanban automation moved this from In progress to Reviewer approved Aug 27, 2021
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Yeah I would separate out the sdk init with the app wrapping.

src/js/sdk.tsx Outdated Show resolved Hide resolved
"@sentry/tracing": "6.7.1",
"@sentry/types": "6.7.1",
"@sentry/utils": "6.7.1",
"@sentry/browser": "6.12.0-beta.2",
Copy link
Member

Choose a reason for hiding this comment

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

We'll make it happen :)

@jennmueng
Copy link
Member Author

jennmueng commented Aug 27, 2021

Can we not do the App wrapping after we init the sdk? This way we can catch errors that occur before we wrap the app?

@AbhiPrasad Init is called right away before we even return the wrapped App:

export function initWith<P>(
RootComponent: React.ComponentType<P>,
passedOptions: ReactNativeWrapperOptions
): React.ComponentType<P> {
const options = _init(passedOptions);
const tracingIntegration = getCurrentHub().getIntegration(ReactNativeTracing);
if (tracingIntegration) {
tracingIntegration.useAppStartWithProfiler = true;
}
const profilerProps = {
...options.profilerProps,
name: RootComponent.displayName ?? "Root",
};
const RootApp: React.FC<P> = (appProps) => {
return (
<TouchEventBoundary {...options.touchEventBoundaryProps}>
<ReactNativeProfiler {...profilerProps}>
<RootComponent {...appProps} />
</ReactNativeProfiler>
</TouchEventBoundary>
);
};
return RootApp;
}

@jennmueng
Copy link
Member Author

@AbhiPrasad I see what you mean now, because this usually wraps the exports this might miss any crashes caused by things you call before the wrapping happens. Let's see how splitting it up goes.

@jennmueng jennmueng changed the title feat: Add initWith method, RN Profiler for better App start, and move Stall Tracking into ReactNativeTracing feat: Add wrap method, RN Profiler for better App start, and move Stall Tracking into ReactNativeTracing Aug 27, 2021
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

wrap is tested via the e2e, right?

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Sweet let鈥檚 ship it

@bruno-garcia bruno-garcia changed the title feat: Add wrap method, RN Profiler for better App start, and move Stall Tracking into ReactNativeTracing feat: wrap method, profiler with App start and Stall tracking Aug 29, 2021
@bruno-garcia bruno-garcia merged commit 9444d71 into master Aug 29, 2021
@bruno-garcia bruno-garcia deleted the jenn/initWith branch August 29, 2021 14:46
Mobile Platform Team Archived automation moved this from In Progress to Done Aug 29, 2021
kanban automation moved this from Reviewer approved to Done Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants