Skip to content

feat: Auto performance tracing with XHR/fetch, and routing instrumentation #1230

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

Merged
merged 40 commits into from
Jan 19, 2021

Conversation

jennmueng
Copy link
Member

@jennmueng jennmueng commented Dec 6, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Note: Needs getsentry/sentry-javascript#3144 to be released over in js before this can be merged.

See App.tsx in the sample as an example of how to initialize auto performance tracing.

Screen Shot 2021-01-10 at 9 49 33 PM

Routing

The routing Instrumentation has to be different than how it's currently implemented for browser given how routing in React Native is handled different and is not global. We would need to create an instance where a route event can be emitted by a navigation router and be listened to by our integration.

Base/Manual Routing Instrumentation

"Manual-Auto"

Create an instance of RoutingInstrumentation, pass it to the tracing integration, and then call onRouteWillChange every time the route changes with the desired transaction context. This sets an idle transaction on the scope.

// Construct a new instrumentation instance. This is needed to communicate between the integration and React
const routingInstrumentation = new Sentry.RoutingInstrumentation();

Sentry.init({
  ...
  integrations: [
    new Sentry.ReactNativeTracing({
      // Pass instrumentation to be used as `routingInstrumentation`
      routingInstrumentation,
      ...
    }),
  ],
})

const App = () => {
  // Call this before the route changes
  routingInstrumentation.onRouteWillChange(transactionContext)
};

React Navigation V5 Instrumentation

Below is an example of how to use the current implementation:

// Construct a new instrumentation instance. This is needed to communicate between the integration and React
const reactNavigationV5Instrumentation = new Sentry.ReactNavigationV5Instrumentation();

Sentry.init({
  ...
  integrations: [
    new Sentry.ReactNativeTracing({
      // Pass instrumentation to be used as `routingInstrumentation`
      routingInstrumentation: reactNavigationV5Instrumentation,
      ...
    }),
  ],
})

const App = () => {
  // Create a ref for the navigation container
  const navigation = React.createRef<NavigationContainerRef>();

  // Register the navigation container with the instrumentation
  React.useEffect(() => {
    reactNavigationV5Instrumentation.registerNavigationContainer(navigation);
  }, []);

  return (
    // Connect the ref to the navigation container
    <NavigationContainer ref={navigation}>
      ...
    </NavigationContainer>
  );
};

Preferably, all the react component part can just be done in a custom hook

React Navigation V4 Instrumentation

Works almost exactly like V5 on the surface, although entirely different underneath.

// Construct a new instrumentation instance. This is needed to communicate between the integration and React
const reactNavigationV4Instrumentation = new Sentry.ReactNavigationV4Instrumentation();

Sentry.init({
  ...
  integrations: [
    new Sentry.ReactNativeTracing({
      // Pass instrumentation to be used as `routingInstrumentation`
      routingInstrumentation: reactNavigationV4Instrumentation,
      ...
    }),
  ],
})

const App = () => {
  // Create a ref for the navigation container
  const appContainer = React.createRef();

  // Register the navigation container with the instrumentation
  React.useEffect(() => {
    reactNavigationV4Instrumentation.registerAppContainer(appContainer);
  }, []);

  return (
    // Connect the ref to the navigation container
    <AppContainer ref={appContainer} />
  );
};

Idle Transaction

The idle transaction is carried over from the browser implementation of tracing. It needs to be exposed upstream from @sentry/tracing. An idle transaction should be started and set on the scope every time a route changes using a routing instrumentation.

Fetch/XHR

Fetch/XHR instrumentation is carried over from the browser implementation, and it also needs to be exposed.

React Profiler

Works out of the box.

https://sentry.io/organizations/sentry-sdks/discover/sentry-react-native:157803b6f5b94f91b5da33014911f181/?field=title&field=event.type&field=project&field=user.display&field=timestamp&name=All+Events&query=&sort=-timestamp&statsPeriod=24h&widths=-1&widths=-1&widths=-1&widths=-1&widths=-1

Need to expose from @sentry/tracing

Some things need to be exposed upstream from @sentry/tracing:

IdleTransaction,
startIdleTransaction,
registerRequestInstrumentation,
RequestInstrumentationOptions,
defaultRequestInstrumentationOptions,

Remaining things to do/investigate

  • Manual Routing Instrumentation
  • React Navigation V5
  • React Navigation V4
  • Fetch/XHR request instrumentation
  • Test Everything on iOS Sim
  • Test Everything on Android Sim
  • Clean up code and comment

💚 How did you test it?

Tested locally on iOS simulator with new sample app.

📝 Checklist

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

🔮 Next steps

@giautm
Copy link

giautm commented Dec 13, 2020

Hi @jennmueng, do you need any help to finish this PR? I'm also awaiting it's go live

@jennmueng
Copy link
Member Author

Hi @jennmueng, do you need any help to finish this PR? I'm so awaiting it's go live

Nothing that would need help, just need time to finish everything and test everything

@jennmueng
Copy link
Member Author

Updated the v5 instrumentation to handle cases where a router with action types other than the default stack ones is used. It now creates an idle transaction on every successful navigation action dispatch, but it does not include route context just yet. It will wait for the navigation state to change, and then update the transaction with the route context. If the state change doesn't happen within 200ms, the transaction is then discarded.

It provides the option of setting a shouldAttachTransaction method that would be called after each state change to determine whether to discard this transaction or keep it. By default it sets a transaction on every new route that isn't a repeat of the last route.

@jennmueng
Copy link
Member Author

Update: Working perfectly and ready for iOS, however I've run into an issue on Android where the current SDK (3.2.0) does not fully support transaction types. Currently going to try a workaround.

@jennmueng
Copy link
Member Author

Temporary Hack to use the js FetchTransport to send transactions on Android for now. This will not be GA and instead just for an alpha. We will need to wait for transactions on FileObserver envelope support on Android

@jennmueng jennmueng marked this pull request as ready for review December 28, 2020 10:50
@jennmueng jennmueng requested a review from a team December 28, 2020 10:50
@jennmueng jennmueng marked this pull request as draft December 29, 2020 18:18
@jennmueng
Copy link
Member Author

Now with instrumentation for React-Navigation V4. It one works differently than the V5 one on the inside, although extremely similar on the outside. This time we patch the router on the AppContainer.

@jennmueng jennmueng marked this pull request as ready for review January 2, 2021 15:56
@jennmueng
Copy link
Member Author

Blocked by getsentry/sentry-javascript#3144 over in JS as everyone there is out of office.

@bruno-garcia
Copy link
Member

Maybe worth resolving conflict with master so it's ready to merge

@jennmueng
Copy link
Member Author

Maybe worth resolving conflict with master so it's ready to merge

Sure, but it would also need a bump to the @sentry/javascript dependencies too before we can merge it btw.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2021

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against aa0786c

@jennmueng
Copy link
Member Author

Just a note: The CI is failing due to things from @sentry/tracing not being exported yet. Need to wait for a new release over at JS before these will pass.

@janicduplessis
Copy link
Contributor

Probably out of scope for this PR, but are there plans to look into also instrumenting the native side? I'm pretty sure RN has hooks for this. This way it would be possible to visualize the full rendering, including react js render, RN bridging, layout and native render.

@bruno-garcia
Copy link
Member

Probably out of scope for this PR, but are there plans to look into also instrumenting the native side? I'm pretty sure RN has hooks for this. This way it would be possible to visualize the full rendering, including react js render, RN bridging, layout and native render.

I dunno if @jennmueng already looked at it, but could you point us to the hooks we can use for that?

We're adding performance API to the native layers next, but we don't have a story for propagating transaction through the bridge yet.

I'm hoping that at least a span for the bridge call will be possible even without any support from the native SDKs.

@jennmueng
Copy link
Member Author

@janicduplessis Yes we're definitely looking into instrumenting React Native way more in the near future. We would really appreciate if you know any of these hooks that you mentioned to see if we can instrument something there.

@janicduplessis
Copy link
Contributor

On Android there's this gist that shows how to use the ReactMarker API https://gist.github.com/axemclion/f01cbb32fcb5b14bf6b4fd4594192825#file-perflogger-java, which should have some interesting info. Not sure about what is available on iOS. Are you on RN contributor discord? There are probably people there that can help too.

@bruno-garcia
Copy link
Member

@janicduplessis Thanks for the link! Not sure about @jennmueng, I'm not on that discord but I'm on the #react-native channel of Sentry's Discord server, would you like to join and we discuss this there?

@jennmueng
Copy link
Member Author

jennmueng commented Jan 11, 2021

Notes: Can use tracesSampler instead of the shouldSendTransaction.

If the app crashes on iOS the transaction might not be stored as it does not have a level = fatal

-> What happens on an errored promise in fetch/xhr if there is a span

@jennmueng jennmueng merged commit 0d1c22c into master Jan 19, 2021
@jennmueng jennmueng deleted the jenn/auto-tracing branch January 19, 2021 11:34
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