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

New debug panel with XVIZ Parser and Log Viewer stats #306

Merged
merged 6 commits into from
May 4, 2019

Conversation

marionleborgne
Copy link
Contributor

Added a new debug panel with @xviz/parserworker farm status + LogViewer stats chart:

Screen Shot 2019-04-28 at 9 37 55 PM

Note: an XVIZWorkersMonitor instance (called monitor) keeps track of the XVIZ worker farm status. When monitor.update() is called, the workers status gets updated. This method is passed as a callback to @xviz/parser's parseStreamMessage via the debug callback argument. When the debug callback is called, the XVIZ workers status are updated in the monitor. This allows us to decorrelate the rate at which the monitor workers status gets updated (on each message by parseStreamMessage) from the rate at which a view component is updated (here XvizWorkersStatus via the monitor's reportCallback). We don't want the view component to re-render on every incoming un-parsed XVIZ message, this would be too heavy.

@@ -61,3 +61,8 @@ export {default as _XVIZLoaderInterface} from './loaders/xviz-loader-interface';
export {default as XVIZStreamLoader} from './loaders/xviz-stream-loader';
export {default as XVIZLiveLoader} from './loaders/xviz-live-loader';
export {default as XVIZFileLoader} from './loaders/xviz-file-loader';

// Debug
export {XvizWorkersStatus} from './debug/xviz-workers-status';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use all caps for XVIZ

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, at least for exports. We have a style guide for XVIZ to be either all upper or all lower, though I can't tell you where it's documented ;)

@@ -102,7 +103,7 @@ export default class XVIZWebsocketLoader extends XVIZLoaderInterface {
message: message.data,
onResult: this.onXVIZMessage,
onError: this.onError,
debug: this._debug.bind('parse_message'),
debug: this.options.debug || this._debug.bind('parse_message'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want to send the 'parse_message' on the _debug object regardless of options.debug

@twojtasz
Copy link
Contributor

left a few inline comments.

Copy link
Contributor

@jlisee jlisee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible in the test app to have a toggle to umount the component (the HUD too would be awesome) below the "show tooltip" one? I find when profiling it's very nice to be able to remove most of the react render calls so you more clearly see the deck.gl/streetscape issue after you have used the debug panel identify a slow down.

@marionleborgne
Copy link
Contributor Author

@jlisee Sure. Like this?

Screen Shot 2019-04-29 at 4 45 51 PM

Screen Shot 2019-04-29 at 4 45 43 PM

@marionleborgne
Copy link
Contributor Author

@jlisee and @twojtasz : addressed your feedback. See last 3 commits.

@twojtasz twojtasz self-requested a review May 1, 2019 17:55
Copy link
Contributor

@twojtasz twojtasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

debug: payload => {
this._debug.bind('parse_message');
if (typeof this.options.debug === 'function') {
this.options.debug(payload);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._debug is the same as this.options.debug with empty fallback.

@@ -102,7 +103,12 @@ export default class XVIZWebsocketLoader extends XVIZLoaderInterface {
message: message.data,
onResult: this.onXVIZMessage,
onError: this.onError,
debug: this._debug.bind('parse_message'),
debug: payload => {
this._debug.bind('parse_message');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a bug in the existing code. Should be debug: this._debug.bind(this, 'parse_message')

@marionleborgne marionleborgne merged commit 23c18bd into aurora-opensource:master May 4, 2019
@marionleborgne marionleborgne deleted the debug-panel branch May 4, 2019 04:07
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.

4 participants