-
Notifications
You must be signed in to change notification settings - Fork 48.5k
interaction-tracking package #13234
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
interaction-tracking package #13234
Conversation
Details of bundled changes.Comparing: e020408...6e31ec3 interaction-tracking
Generated by 🚫 dangerJS |
0cf6399
to
9624848
Compare
}; | ||
} | ||
|
||
export function startContinuation(context: ZoneContext): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The start/stop continuation methods aren't really intended for user-facing use. They enable React to restore previous context when processing work asynchronously.
While testing the new DevTools profiler, I noticed that sometimes– in larger, more complicated applications– the actualDuration value was incorrect (either too large, or sometimes negative). I was not able to reproduce this in a smaller application or test (which sucks) but I assume it has something to do with the way I was tracking render times across priorities/roots. So this PR replaces the previous approach with a simpler one.
5a3df8d
to
8df2067
Compare
Feedback from the Web Speed team has been incorporated. A new module, The only open question (that I can think of at the moment) is how we can change continuations to accommodate the fact that multiple interactions might get batched into a single React commit. The current API assumes that you'll only restore a single interaction context (a "continuation"). |
I think I've responded to all of the feedback about this PR 👍 |
} | ||
} | ||
|
||
if (caughtError !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this would swallow throw null
.
In React, we solve this by having a wrapper function that toggles a boolean if an error was set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. That occurred to me but seemed unlikely. I'll replace it with a bool though!
PR has been updated with the following changes:
Should be ready for final review/merge now, since nothing actually references this new package until PR #13253 (and the feature flags are all off by default). cc @acdlite @sebmarkbage 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Mostly a bunch of nits.
return callback(); | ||
} | ||
|
||
const interaction: Interaction = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will provide a hint to the VM that we only want 3 inline fields but they you add an expando later.
We should always provide the four fields in initialization. If you want to exclude on when the feature flag is not used you can initialize the object in a conditional.
In the three field case you can cast it to any and then back to Interaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good call. Thanks!
import now from './InteractionTrackingNow'; | ||
|
||
export type Interaction = {| | ||
__count?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be required.
I realize it won’t be there in one of the feature flags but we can assume it always is. That way you don’t need all the any casts whenever you access this which might cover up other issues.
// Interactions "stack"– | ||
// Meaning that newly tracked interactions are appended to the previously active set. | ||
// When an interaction goes out of scope, the previous set (if any) is restored. | ||
let interactionsRef: InteractionsRef | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can cast the null to any so that this can be non-nullable. That way the fact that this is dependent on feature flags is hidden in one place instead of spread throughout the code.
Every use of any could be a typo/miscast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting.
Using "any" to mask a null initialization value seems equally potentially dangerous but... I don't mind removing all of the Flow casts 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both cases has the danger of us using a field while not in the feature flag branch.
In the previous approach, every time you use it risk typing (interactionsRef: any)
instead of ((interactionsRef: any): InteractionsRef)
which would cover up additional errors.
You also risk typing ((subscriberRef: any): InteractionsRef)
which is just the wrong type.
The number of times you have to use any
increases the risk of any of these types of additional mistakes.
When it is in a single location you only risk making this mistake once, but that's easy to review, and since you're using null
as the initial value the risk of typing subscriberRef
instead of null
seems pretty small.
|
||
// Listener(s) to notify when interactions begin and end. | ||
// Note that subscribers are only supported when enableInteractionTrackingObserver is enabled. | ||
let subscriberRef: SubscriberRef | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
returnValue = callback(); | ||
} catch (error) { | ||
didCatch = true; | ||
caughtError = caughtError || error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you throw null in the first one, and this also throws, it’ll override with the second error. Not sure if that’s desireable. We can make this if (!didCatch) { didCatch = true; caughtError = error; }
. That way we don’t have to rely on the sketchy falsey check. It will always rethrow the first throw value just like try finally.
interaction.__count = 1; | ||
|
||
let caughtError; | ||
let didCatch = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you just use nested try/finally where each next level is in the finally, I don’t think you need to use these temporary variables or any catch blocks. That’d probably be better perf wise too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm...I could do this, but it would end up throwing the last error, rather than the first. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. You're right. It might be worth it though. Just for smaller code and doesn't require the VM to set up registers and initialize these values. It becomes a noop. We don't expect the handlers to ever throw so optimizing for that seems valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okedoke. I'll change it.
|
||
try { | ||
let caughtError; | ||
let didCatch = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here. You can just use nested try/finally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto ^
const interaction: Interaction = { | ||
id: interactionIDCounter++, | ||
name, | ||
timestamp: now(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is nothing else in here that deals with timestamps, it seems better that this is passed in instead. That way the time mechanism doesn’t have to be built in. Eg we are considering a custom measurer at FB and RN might need something else.
In fact for certain tracking I believe that is required. Eg for a click you want the time at the time the user released. Not towards the end of the processing of the click event. The mouseup event could’ve spent time too. For requestAnimationFrame you want the time stamp passed into the callback (beginning of the frame), not the current time.
Additionally we don’t even know which scale is appropriate to use. Date.now and performance.now use different time scales and whatever they’re going to be compared to (eg) commit time needs to line up.
If we’re going to automatically track time here, then we should also pass time to the complete callbacks so that the external system gets consistent time. But I see no reason for that.
We can just have the user of the track api pass the current time in as an argument. That makes this package a bit lighter too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...that would make the API feel clunkier to me
That makes this package a bit lighter too.
By such a tiny amount that it isn't really important 😁
((subscriberRef: any): SubscriberRef).current === null || | ||
((subscriberRef: any): SubscriberRef).current === subscriber, | ||
'Only one interactions subscriber may be registered at a time.', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only one can be scheduled at a time, maybe the API should be to mutate current
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that is what React does. Why does this other API exist at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React mutates the interactionRef
directly. Non-react things (e.g. Web Speed's code) will subscribe. I don't really want to expose the ref to things we don't directly control.
import invariant from 'shared/invariant'; | ||
import { | ||
enableInteractionTracking, | ||
enableInteractionTrackingObserver, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t get why these feature flags exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rather, I get why they exist for React but not why they are used by this package. Is it because we’re still inlining it instead of publishing as a separate module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enableInteractionTracking
is used to make the production bundle no-op for interaction tracking (since nothing consumes that data).
The enableInteractionTrackingObserver
flag is maybe not necessary, but I was using it when the subscription stuff was more obnoxious so that we could turn it off entirely if we decided to (e.g. for open source). I could remove this flag if we want.
export function wrap( | ||
callback: Function, | ||
threadID: number = DEFAULT_THREAD_ID, | ||
): Function { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand why wrap
is so complicated. I was expecting it to be essentially this:
function wrap(callback) {
const continuedInteractions = interactions.current;
retain();
return (...args) => {
track(continuedInteractions, () => callback(...args));
release();
}
}
where most of the code is reused from track
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose avoiding the extra function call is worthwhile, but I believe semantically they should be identical. See other comment: #13234 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think wrap is that complicated. This approach avoids an extra function call and additional wrapper function.
Also the behavior of wrap/track are different. track
is additive (stacking on top of what the current interactions are). wrap
temporarily restores interactions at a previous point in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But... why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because that's kind of fundamental to how track
and wrap
work? I'm not sure I understand the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn’t modify the current set of interactions because it will reset them back to the previous set after it exits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I meant that it would modify the original set. So if I wrapped [foo,bar] I would expect my async work to be attributed to [foo,bar]– not [foo,bar,...whatever-else-was-also-active]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @acdlite's misunderstanding is a good opportunity to rename this function to something that makes it clearer. I could see how if I see track
and wrap
I would think that wrap
is just a convenience for wrapTracked
since it is highlighting the wrapping mechanism and nothing else.
In reality the purpose of wrap isn't to wrap a function. The primary purpose is to continue where you left off.
wrapContinuation
? Too long. This is can be used a lot if you don't have auto-wrapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose there's precedence for calling it wrap
in Zones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the brevity of "track" and "wrap" but I'm not bullish on them if others feel there are more meaningful names.
|
||
const wrapped = (...args) => { | ||
const prevInteractions = ((interactionsRef: any): InteractionsRef).current; | ||
((interactionsRef: any): InteractionsRef).current = wrappedInteractions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost always, this callback will be called at or near the top of the stack. But if it isn't, shouldn't this "stack/accumulate" interactions the same way track
does? (If so, why not?) This is one of the reasons I assumed you would implement wrap
on top of track
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My above comment mentions this. The behavior of wrap/track are intentionally different. track
is additive (stacking on top of what the current interactions are). wrap
temporarily restores interactions at a previous point in time.
} | ||
} else { | ||
try { | ||
return callback(...args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think this is fine and Babel will compile it to callback.apply(undefined, arguments)
but to be safe I usually prefer to manually use apply
.
* Refactored some nullable Flow types * Moved __count into interactions inline object rather than adding field later * Changed didCatch condition to better handle thrown null value
I believe I've made all of the requested changes, except for the nested try/finally suggestion from Sebastian– since that would change throwing behavior. |
69ab7c7
to
cc45d8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand how the feature flags work and how they're going to work to enable this. Will you need to alias the production build of this package too, or just ReactDOM to enable tracking?
But I'm satisfied.
} | ||
} finally { | ||
try { | ||
returnValue = callback(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can actually just return here. The finally will still execute before returning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning in finally masks thrown errors.
"name": "interaction-tracking", | ||
"description": "utility for tracking interaction events", | ||
"version": "0.0.1", | ||
"repository": "facebook/react", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to flush this package.json out a bit.
I could roll the interaction tracking feature flags into the In general, the question of "how to use this in production" is the same for this package as for the profiling bundle of e.g. react-dom– and yeah, I assume you would just setup an alias in your bundler. |
return ++threadIDCounter; | ||
} | ||
|
||
export function subscribe(subscriber: Subscriber): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of agree with @acdlite here.
We could have a separate module. interaction-tracker/observer
that attaches itself to __subscriberRef
directly but provides multi-listener support.
That could be the safe API. That way the minimal module remains even tinier by excluding the overhead of exposing these methods and this error message.
It's never really safe to listen to a single listener protocol directly regardless. The safe form is the multi-listener form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll pull the subscribe
/unsubscribe
methods for now and re-add them in a follow up.
Thanks, everyone, for the thoughtful review! ❤️ I'm going to land this and do the following with a follow up PR:
I'll then rebase and flatten PR #13253 before starting to respond to feedback. |
Backstory
We've recently introduced an experimental profiler API along with a prototype DevTools integration. After discussing this API with a few teams at Facebook, one common piece of feedback is that the timing information is useful, but would be more useful if it could be associated with "events" (e.g. button click, XHR response).
Event tracking would enable more powerful tooling to be built around the timing information, capable of answering questions like "What caused this really slow commit?" or "What's the wall time for this interaction?". It would also enable the DevTools Profiler plug-in to show a more meaningful, event-based timeline view.
This PR introduces a new API for tracking interactions. PR #13253 integrates this package with React and the experimental profiler API.
High level goals
Tracking interactions
The tracking API is conceptually similar to zones.
track
is called to register a new event, and work is done in a callback. React automatically associates the work done in the callback with the current event(s) and passes them to the profiler'sonRender
callback when the work is eventually committed.For example, you might track an event for (re)rendering your application:
Or you might track a user interaction that triggers a state-update:
If your application tree has a profiler component (
React.unstable_Profiler
) then the tracked information will be passed to itsonRender
callback as an additional array param. That information might look something like this:Basic API
clear(callback: Function) => any
Resets the interaction stack temporarily, allowing new work to be tracked without appending to previous interactions. The callback function will be executed and its return value will be returned to the caller.
getCurrent() => Set<Interaction>
Returns the current set of interactions.
track(name: string, callback: Function, threadID: number = 0) => any
Tracks a new interaction (by appending to the existing set of interactions). The callback function will be executed and its return value will be returned to the caller. Any code run within that callback will be within that interaction "zone". Calls to
wrap()
will schedule async work within the same zone.A
threadID
can be passed to specify who is performing the current work. This value defaults to 0 which can be thought of as the global "thread".wrap(callback: Function, threadID: number = 0) => Function
Wrap a function for later execution within the current interaction "zone". When this function is later run, interactions that were active when it was "wrapped" will be reactivated.
The wrapped function returned also defines a
cancel()
property which can be used to notify any tracked interactions that the async work has been cancelled.A
threadID
can be passed to specify who is performing the current work. This value defaults to 0 which can be thought of as the global "thread".Advanced API
By default, the interaction-tracking package only manages interaction "zones". In order to build more complex logic on top of this package, it also supports an subscrible API. Subscribers can be added or removed using
subscribe()
andunsubscribe()
methods. A subscriber should implement the following API:onInteractionTracked(interaction: Interaction) => void
A new interaction has been created via the track() method.
onInteractionScheduledWorkCompleted(interaction: Interaction) => void
All scheduled async work for an interaction has finished.
onWorkScheduled(interactions: Set<Interaction>, threadID: number) => void
New async work has been scheduled for a set of interactions. When this work is later run, onWorkStarted/onWorkStopped will be called.
A batch of async/yieldy work may be scheduled multiple times before completing. In that case, onWorkScheduled may be called more than once before onWorkStopped.
Work is scheduled by a "thread" which is identified by a unique ID.
onWorkStarted(interactions: Set<Interaction>, threadID: number) => void
A batch of work has started for a set of interactions. When this work is complete, onWorkStopped will be called. Work is not always completed synchronously; yielding may occur in between.
A batch of async/yieldy work may also be re-started before completing. In that case, onWorkStarted may be called more than once before onWorkStopped.
Work is done by a "thread" which is identified by a unique ID.
onWorkCancelled(interactions: Set<Interaction>, threadID: number) => void
A batch of scheduled work has been cancelled.
Work is done by a "thread" which is identified by a unique ID.
onWorkStopped(interactions: Set<Interaction>, threadID: number) => void
A batch of work has completed for a set of interactions.
Work is done by a "thread" which is identified by a unique ID.