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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make MessageQueue to emit "SPY" events in a way that can be extensible #9160

Closed
wants to merge 6 commits into from

Conversation

@jondot
Copy link
Contributor

jondot commented Aug 2, 2016

This PR adds a capability for MessageQueue to emit "SPY" events in a way that can be extensible, to later allow for a tooling ecosystem to grow, one example is the existing Snoopy tool that is, for now, forced to work with monkeypatches, and after this PR will be able to use a "formal" way to trace queue events.

After this change, we can wire a "spy" into a queue that will expose the events in different and interesting ways, see below (done with Snoopy):
Aggregating and Charting Events with Bar
Aggregating and Charting Events with Bar

This removes the hardcoded SPY_MODE flag and instead uses a function that can be injected from outside world.

MessageQueue.spy((info)=>console.log("event!", info)

It also creates a structured descriptor for an event, so in the above example, console.log will accept the following object shape:

{
   type: integer, (0:TO_JS, 1:TO_NATIVE)
   module:string,
   method:string,
   args:[object]
}

Once a spy exists (and we're in __DEV__ mode), the queue will start using it.

In terms of core changes - I feel this was a low-hanging-fruit, small enough to pick.

jondot added 2 commits Jul 27, 2016
in a way that can be extensible.

After this change, we can wire a "spy" into a queue
that will expose the events in different and interesting
ways. This removes the hardcoded `SPY_MODE` flag and
instead uses a function that can be injected from outside.

This change will enable a tool like Snoopy to exist,
without its internal monkeypatching of MessageQueue:

https://github.com/jondot/rn-snoopy

This was a low-hanging-fruit waiting to be picked. And
now, we have deeper and more elaborate introspection
at the React Native bridge traffic, empowering optimization
and debugging scenarios alike.
@ghost
Copy link

ghost commented Aug 2, 2016

By analyzing the blame information on this pull request, we identified @javache and @lexs to be potential reviewers.

@@ -26,9 +26,10 @@ const METHOD_IDS = 1;
const PARAMS = 2;
const MIN_TIME_BETWEEN_FLUSHES_MS = 5;

const TRACE_TAG_REACT_APPS = 1 << 17;
const TO_NATIVE = 1
const TO_JS = 0

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 2, 2016

semi: Missing semicolon.

@@ -26,9 +26,10 @@ const METHOD_IDS = 1;
const PARAMS = 2;
const MIN_TIME_BETWEEN_FLUSHES_MS = 5;

const TRACE_TAG_REACT_APPS = 1 << 17;
const TO_NATIVE = 1
const TO_JS = 0

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 2, 2016

semi: Missing semicolon.

@facebook-github-bot
Copy link

facebook-github-bot commented Aug 2, 2016

@jondot updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Aug 2, 2016

@jondot updated the pull request.

@ghost ghost added the CLA Signed label Aug 2, 2016
@@ -182,18 +188,22 @@ class MessageQueue {
this._lastFlush = now;
}
Systrace.counterEvent('pending_js_to_native_queue', this._queue[0].length);
if (__DEV__ && SPY_MODE && isFinite(module)) {
console.log('JS->N : ' + this._remoteModuleTable[module] + '.' +

This comment has been minimized.

Copy link
@javache

javache Aug 3, 2016

Member

Can you keep the original console.log feature around somewhere?

This comment has been minimized.

Copy link
@jondot

jondot Aug 3, 2016

Author Contributor

Yes!
I'm thinking perhaps a default 'spy' implementation, that you can use and is built-in.
Something like I did with this and this.

The ergonomics would look like this:

MessageQueue.spy()

sets the default logging strategy in place.

Then clearing a spy, disables tracing, and would have to be done like so

MessageQueue.spy(null)

WDYT?

This comment has been minimized.

Copy link
@javache

javache Aug 3, 2016

Member

Maybe MessageQueue.spy(true) to turn on the default function, MessageQueue.spy(fn) to provide a custom function and MessageQueue.spy(false) to turn it off?

This comment has been minimized.

Copy link
@jondot

jondot Aug 3, 2016

Author Contributor

sounds even better, thanks for the idea!

This comment has been minimized.

Copy link
@jondot

jondot Aug 3, 2016

Author Contributor

Changes are in!

@facebook-github-bot
Copy link

facebook-github-bot commented Aug 3, 2016

@jondot updated the pull request.

const TRACE_TAG_REACT_APPS = 1 << 17;
const TO_NATIVE = 1;
const TO_JS = 0;
const defaultSpy = (info)=>console.log(`${info.type == TO_JS ? 'N->JS' : 'JS->N'} : ${info.module ? (info.module+'.') : ''}${info.method}(${JSON.stringify(info.args)})`);

This comment has been minimized.

Copy link
@javache

javache Aug 3, 2016

Member

Can you split this over multiple lines? Since it's also only used in one spot, I'd just define it inline in spy

This comment has been minimized.

Copy link
@jondot

jondot Aug 3, 2016

Author Contributor

yup!

@@ -90,6 +92,17 @@ class MessageQueue {
/**
* Public APIs
*/

static spy(spyOrToggle){
if(spyOrToggle === true){

This comment has been minimized.

Copy link
@javache

javache Aug 3, 2016

Member

style nit: if (spyOrToggle === true)

This comment has been minimized.

Copy link
@jondot

jondot Aug 3, 2016

Author Contributor

👍

@@ -448,4 +465,6 @@ function lazyProperty(target: Object, name: string, f: () => any) {
});
}


This comment has been minimized.

Copy link
@javache

javache Aug 3, 2016

Member

Remove these

This comment has been minimized.

Copy link
@jondot

jondot Aug 3, 2016

Author Contributor

👍

@@ -90,6 +92,17 @@ class MessageQueue {

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 3, 2016

keyword-spacing: Expected space(s) after "if".

@@ -90,6 +92,17 @@ class MessageQueue {
/**
* Public APIs

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 3, 2016

keyword-spacing: Expected space(s) after "if".

@@ -26,9 +26,11 @@ const METHOD_IDS = 1;
const PARAMS = 2;
const MIN_TIME_BETWEEN_FLUSHES_MS = 5;

const TRACE_TAG_REACT_APPS = 1 << 17;

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 3, 2016

eqeqeq: Expected '===' and instead saw '=='.

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 3, 2016

space-infix-ops: Infix operators must be spaced.

@ghost
Copy link

ghost commented Aug 3, 2016

@jondot updated the pull request.

@ghost ghost added the CLA Signed label Aug 3, 2016
@@ -90,6 +91,19 @@ class MessageQueue {
/**
* Public APIs

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 3, 2016

eqeqeq: Expected '===' and instead saw '=='.

@@ -90,6 +91,19 @@ class MessageQueue {
/**
* Public APIs
*/

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 3, 2016

space-infix-ops: Infix operators must be spaced.


static spy(spyOrToggle){
if (spyOrToggle === true){
MessageQueue.prototype.__spy = (info)=>console.log(`${info.type == TO_JS ? 'N->JS' : 'JS->N'} : ` +

This comment has been minimized.

Copy link
@javache

javache Aug 3, 2016

Member

Can you split this over multiple lines?

MessageQueue.prototype.__spy = (info) => {
  console.log(...)
};

This comment has been minimized.

Copy link
@jondot

jondot Aug 4, 2016

Author Contributor

Yup, thanks!

@facebook-github-bot
Copy link

facebook-github-bot commented Aug 4, 2016

@jondot updated the pull request.


const SPY_MODE = false;
const TRACE_TAG_REACT_APPS = 1 << 17;

const MethodTypes = keyMirror({

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 4, 2016

semi: Missing semicolon.


const SPY_MODE = false;
const TRACE_TAG_REACT_APPS = 1 << 17;

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 4, 2016

space-infix-ops: Infix operators must be spaced.


const SPY_MODE = false;

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Aug 4, 2016

eqeqeq: Expected '===' and instead saw '=='.

@ghost ghost added the CLA Signed label Aug 4, 2016
@javache
Copy link
Member

javache commented Aug 4, 2016

@facebook-github-bot shipit

@ghost
Copy link

ghost commented Aug 4, 2016

Thanks for importing.

@ghost ghost added the CLA Signed label Aug 4, 2016
@ghost ghost closed this in 77e48f1 Aug 4, 2016
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
This PR adds a capability for MessageQueue to emit "SPY" events in a way that can be extensible, to later allow for a tooling ecosystem to grow, one example is the existing [Snoopy](https://github.com/jondot/rn-snoopy) tool that is, for now, forced to work with monkeypatches, and after this PR will be able to use a "formal" way to trace queue events.

After this change, we can wire a "spy" into a queue that will expose the events in different and interesting ways, see below (done with Snoopy):
  <img src="https://github.com/jondot/rn-snoopy/blob/master/media/snoopy.gif?raw=true" alt="Aggregating and Charting Events with Bar" width="400px"/>
  <img src="https://github.com/jondot/rn-snoopy/blob/master/media/snoopy-filter.gif?raw=true" alt="Aggregating and Charting Events with Bar" width="400px"/>

This removes the hardcoded `SPY_MODE` flag and instead uses a function that can be injected from outside world.

```javascript
MessageQueue.spy((info)=>console.log("event!", info)
```

It also creates
Closes facebook#9160

Differential Revision: D3669053

Pulled By: javache

fbshipit-source-id: 3e4462aa77fc8514d2ea4f15430f7bec57b583a4
jondot added a commit to jondot/rn-snoopy that referenced this pull request Sep 26, 2016
see: facebook/react-native#9160

Bump version so that it's visibly clear this is React Native 0.33 and up
(v1.33.0)

Update example and make notes of this in README.
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.