Interaction Tracking -- Observer context switching lifecycle. #10

salazarm · 2018-08-02T20:22:39Z

This PR builds on PR facebook#13234 which Introduces an experimental Interaction Tracking API.

The goal of this PR is to add a lifecycle around the context switching that happens inside of Interaction Tracking so that we can implement a profiler that can reconstruct the DAG of execution (InteractionProfiler.js)

The changes in this PR are:

Make the ZoneContext type an actual Object rather than ANY type. This was done primarily so that we could have an ID field that observers could use to identify execution blocks within the same instance of an interaction.
Added a Lifecycle around context switching onContextScheduled, onContextStarting, onContextEnded.
Added a getSettableContext method to be used in conjunction with startContinuation. getSettableContext calls the onContextScheduled lifecycle which will trigger our Observer to increment the reference count for the current interaction contexts.

…t into interaction-tracking-marco

salazarm · 2018-08-02T20:54:48Z

Forgive my poor commit hygiene, lets comment on the Files Changed tab rather than on commits =O

bvaughn

Left some initial thoughts. Let's chat again after you've had a chance to read them.

By the way, you'll need to fetch and pull the branch you're based on. It's outdated and there are conflicts.

git fetch bvaughn
git rebase bvaughn/interaction-tracking

bvaughn · 2018-08-03T14:41:53Z

packages/interaction-tracking/src/InteractionContextEmitter.js

+   ) => void;
+ }
+
+const observers: Array<InteractionContextObserver> = [];


The creation of this Array should be done within a __PROFILE__ conditional so it's only included in development and profiling builds (and not production).

bvaughn · 2018-08-03T14:42:31Z

packages/interaction-tracking/src/InteractionContextEmitter.js

+
+export function registerInteractionContextObserver(
+  observer: InteractionContextObserver,
+): void {


Similarly, these methods should all be no-ops for production builds, e.g.:

export function registerInteractionContextObserver( observer: InteractionContextObserver, ): void { if (!__PROFILE__) { return; } observers.push(observer); }

bvaughn · 2018-08-03T14:43:57Z

packages/interaction-tracking/src/InteractionProfiler.js

+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ */


I don't think this module should be part of the interaction-tracking package. Seems like it should be part of a separate package that depends on interaction-tracking?

Yes correct, this was included as a proof of concept just to show how we'd use the API. I'll remove it.

bvaughn · 2018-08-03T14:45:39Z

packages/interaction-tracking/src/InteractionTracking.js

-  startZoneContinuation(interactions);
+let contextIDDispenser: number = 0;
+
+export function startContinuation(context: SettableContext | null): void {


~~This changed from an array of Interactions to a single SettableContext. That seems wrong to me.~~ Whoops, disregard this comment. I meant to delete it.

bvaughn · 2018-08-03T14:49:03Z

packages/interaction-tracking/src/InteractionTracking.js

+  }
+}
+
+export function registerInteractionContextObserver(observer) {


Both getSettableContext and registerInteractionContextObserver could just be exported as-is from InteractionZone. If you update the version of my branch this PR is based on, that's what I'm doing for all but the track function (since it adds a small amount of additional behavior).

Or maybe we should just merge the two modules at this point. Especially since this PR has kind of coupled the zone's context type with Interaction.

bvaughn · 2018-08-03T14:53:28Z

packages/interaction-tracking/src/InteractionZone.js

@@ -64,6 +107,12 @@ export function startContinuation(context: ZoneContext | null): void {
  );
  continuationContext = context;
  isInContinuation = true;
+  invariant(


super tiny nit: Seems like this invariant belongs at the beginning of the function with the other one.

bvaughn · 2018-08-03T14:53:45Z

packages/interaction-tracking/src/InteractionZone.js

+  return ({
+    __context: context,
+    __id: id,
+    __startCount: 0,


nit: Can't this just be a boolean?

bvaughn · 2018-08-03T14:55:19Z

packages/interaction-tracking/src/InteractionZone.js

+}
+
+let continuationContext: SettableZoneContext | null = null;
+let currentContext: Array<ZoneContext> | null = null;
 let isInContinuation: boolean = false;


isInContinuation was only necessary when continuationContext was a nullable param to startContinuation. If it's non-nullable, we can remove isInContinuation and just check continuationContext !== null in our invariants.

bvaughn · 2018-08-03T14:58:08Z

packages/interaction-tracking/src/InteractionZone.js

+  const context = isInContinuation ? continuationContext : currentContext;
+  const id = ++executionID;
+  __onWrappedContextScheduled(context, id);
+  return ({


The SettableZoneContext.__context type Array<ZoneContext> doesn't seem to line up with the fact that ZoneContext might be null. I think this function needs to check if context is null and just return null too if it is? I'm surprised Flow didn't catch this, unless I'm reading it wrong.

bvaughn · 2018-08-03T14:58:48Z

packages/interaction-tracking/src/__tests__/InteractionTracking-test.js

-
-            InteractionTracking.startContinuation(null);
-            expect(InteractionTracking.getCurrentEvents()).toEqual(null);
-            InteractionTracking.stopContinuation();


Why'd you remove these tests? I think they're still useful.

You can't pass null to startContinuation anymore. We'll need some sort of unwrap method to accomplish what is attempted here.

Hm. I think there's still a disconnect in nullability as my other comment mentioned. I'll check it out when I'm merging/rebasing.

bvaughn · 2018-08-03T17:33:53Z

packages/interaction-tracking/src/InteractionZone.js

@@ -42,19 +62,42 @@ export function wrap(callback: Function): Function {
  }

  const wrappedContext = currentContext;
+  const id = ++executionID;


What's the value in differentiating the execution id and the context/interaction id(s)? It's fine, I'm just curious what it lets us do.

the execution ID allows us to pair up calls of onContextScheduled/Starting/Ended. Check out how InteractionProfiler is using the ID.

bvaughn · 2018-08-03T22:23:44Z

The introduction of SettableContext also has tricky implications when it comes to calling track() while inside of a "continuation". track() won't actually mask the continuation in that case because of how "get current" always gives priority to the continuation.

I think the solution is to disallow track() calls when we're within a continuation. (Or to change the way continuation masking works entirely...)

bvaughn · 2018-08-03T22:35:38Z

packages/interaction-tracking/src/InteractionZone.js

+  if (!__PROFILE__) {
+    return null;
+  }
+  const context = isInContinuation ? continuationContext : currentContext;


Something about this seems wrong, but I might be getting your PR– and my updated branch– mixed up...but if getSettableContext() is called while in a continuation– and this function returns continuationContext– won't that cause an error the next time it's called?

I'll try to tackle this when merging things, but we may need to revisit this question.

bvaughn · 2018-08-04T20:59:56Z

Okay. I merged my branch into this branch and addressed my own PR feedback. I am pretty satisfied with where we arrived.

bvaughn · 2018-08-04T21:03:42Z

I'm going to move forward with this so I can start updating the integration PR (facebook#13253) but let's circle back and talk on Monday to address any concerns or feedback you have.

bvaughn · 2018-08-04T21:22:40Z

Gah, actually– we'll still need to revisit continuations. Since multiple interactions might get batched into a single React commit, React will need the ability to continue multiple interactions at once.

I think this could be made to mostly work, but is a little tricky when it comes to the concept of an "execution id". Multiple IDs would potentially be "scheduled" individually and then subsequently continued in a batch. I guess the API could just accept an array of continuations to start/stop?

It's also a bit tricky, since React unwinds stacks of interactions in order to store them as a flat list of unique interactions. 🤔

bvaughn · 2018-08-04T22:34:52Z

I also think it would probably be better if the API supported requesting a continuation for a specific interaction because– as it currently stands, React might end up requesting a lot of unnecessary continuations.

bvaughn · 2018-08-05T00:41:05Z

I thought about this a lot on my afternoon run and I think I have some ideas that will work. Stay tuned!

bvaughn · 2018-08-05T19:34:00Z

facebook@14dffc7

* Fixes docker-compose & pg initialization - ./scripts/init_db.sh is not executable (/bin/bash: bad interpreter: Permission denied) - same script fails because user is already created, only table creation is necessary. * Actually fix docker-compose development workflow * Incorporate PR feedback * Remove credentials.json

salazarm added 6 commits July 30, 2018 15:35

Interaction Context Observing for Profiling

3cbab69

Interaction Context Observing for Profiling

42aa0e3

Merge branch 'interaction-tracking-marco' of github.com:salazarm/reac…

113047a

…t into interaction-tracking-marco

some cleanup

54b1c93

cleanup + tests

35fc821

readonly + fix flow types

1490637

bvaughn reviewed Aug 3, 2018

View reviewed changes

Merged interaction-tracking branch and simplified context tracking

34c96f4

bvaughn mentioned this pull request Aug 3, 2018

interaction-tracking package facebook/react#13234

Merged

Brian Vaughn added 2 commits August 4, 2018 13:13

Subjective nits to tidy up continuation tests

49d1e24

Addressed TODO about tracking during a continuation (and added test)

c3662fa

bvaughn merged commit 5a3df8d into bvaughn:interaction-tracking Aug 4, 2018

Uh oh!

Interaction Tracking -- Observer context switching lifecycle. #10

Interaction Tracking -- Observer context switching lifecycle. #10

Uh oh!

Conversation

salazarm commented Aug 2, 2018

Uh oh!

salazarm commented Aug 2, 2018

Uh oh!

bvaughn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Aug 4, 2018

Uh oh!

bvaughn commented Aug 4, 2018

Uh oh!

bvaughn commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn commented Aug 4, 2018

Uh oh!

bvaughn commented Aug 5, 2018

Uh oh!

bvaughn commented Aug 5, 2018

Uh oh!

Uh oh!

bvaughn Aug 3, 2018 •

edited

Loading

bvaughn commented Aug 3, 2018 •

edited

Loading

bvaughn Aug 3, 2018 •

edited

Loading

bvaughn commented Aug 4, 2018 •

edited

Loading