ref: Rewrite React Profiler

[AbhiPrasad]

I rewrote the Profiler, it's much more useful now.

Motivation

After the meeting for performance last Wednesday, I re-evaluated our React Profiler, and decided it wasn't meeting the idea of, "is this useful?"

In my eyes, the Profiler should be looking at three primary things.

1. How long does it take/when does a component mount on a page
2. How long does a component stay on a page (visible/rendered)?
3. How many times did a component update, and why?

It's important to keep in mind that the questions the Profiler is trying to answer should be as lightweight as possible. The React profiler is not aiming to replace dev tools and dev debugging, but instead is used to clue in users about where things are inefficient, or why certain spans happened the way they did.

Implementation

We now recommend only the HOC withProfiler as the way to use the Profiler. This is so the user can pass in the necessary options, and we can correctly intercept the props for the react.update spans.

We now create 3 different spans from a react component, each trying to answer the respective question from above.

1. react.mount - the time between component constructor and it mounting on a page.

If this span was not created, it will not create the following spans. This is the only span that is required, the other one's can be turned off using options passed into the Profiler.

2. react.render - the time a component is on a page.

Only will show up if a component unmounted while the transaction was still occurring. This is a child span to the react.mount span.

3. react.update - if the props had changed in a component during a transaction, we create a span for it.

This will list the changedProps as data on the span. This is a child span to the react.mount span (but I'm not sure if it should be). It is important to note that the useProfiler hook does not generate this span as it cannot access component props (maybe we should allow users to pass it in?).

Results

Testing

This was tested on getsentry using getsentry/sentry#19366 as well as unit tests were updated to account for new functionality.

Future

We are ready to

[HazAT]

Are there cases where we don't have addGlobalEventProcessor? 🤔

[AbhiPrasad]

If a user doesn't mock it out in tests, don't want to create unnecessary friction.

[getsentry-bot]

	Messages
📖	@sentry/browser bundle gzip'ed minified size: (ES5: 17.0986 kB) (ES6: 16.1699 kB)
📖	✅ TSLint passed

Generated by 🚫 dangerJS against 0a32bf5

[billyvg]

This seems like a good change 👍

• Are there any performance concerns for showing changed props?
• I'm curious how our profiler compares to the react profiler api

[AbhiPrasad]

@billyvg I did a bunch of work around experimenting with the profiler API (including the scheduler and interaction tracing bits) - #2660, but decided to leave it due to the reasons described there. Will probably revisit at a certain point (honestly if I might just write an email to someone on the React core team and ask for help :P)

We are not doing any deep equality checks, and try to short circuit the calculation asap, so I think performance impact is minimal. I have checked with local react dev tools and it couldn't really find any significant overhead in the flame graph. If people do complain about this, we can revist this and explore alternate options.

Users are also able to disable this feature by setting hasUpdateSpans = true as an option. This is a good point though, and something we will have to talk about in the docs.

[billyvg]

@AbhiPrasad Yeah I'm more curious about comparing the timings from our profiling vs the timings from react profiler.

I think it would be good to record some profiling/benchmarking for the update spans so that we have something to point users to