[WIP] Add WebSocket connection and activities in the Performance tool [devtools/devtools-frontend : main]
Christy Chen (Gerrit)
Message from Christy Chen![]( "Open in Gerrit")
Set Ready For Review
Related details
- Christy Chen
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christy Chen (Gerrit)
Christy Chen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christy Chen
| Commit-Queue | +1 |
1 comment
Adding websocket connection to Network track, appreciate if you can have a look. Thank you!
Related details
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Irish (Gerrit)
Paul Irish added 1 comment![]( "Open in Gerrit")
Just tried it out using https://territorial.io/
Really like the UI. This is great.
I'll take a deeper look tomorrow. :)
Related details
- Christy Chen
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christy Chen (Gerrit)
Christy Chen added 1 comment![]( "Open in Gerrit")
Just tried it out using https://territorial.io/
Really like the UI. This is great.I'll take a deeper look tomorrow. :)
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christy Chen (Gerrit)
Christy Chen added 1 comment![]( "Open in Gerrit")
Hey Changhao, would you be able to take a look, or route it to an appropriate reviewer? Thank you!
Related details
- Changhao Han
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Changhao Han (Gerrit)
Changhao Han added 1 comment![]( "Open in Gerrit")
Hi Christy, I think Jack would be the expert here to look at this, along with Paul.
Related details
- Christy Chen
- Jack Franklin
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Irish (Gerrit)
Paul Irish added 7 comments![]( "Open in Gerrit")
First up, I realize I basically handed you an impossible task.
"Let's have the instant events be part of a larger websocket event!"
As if that's trivial. :p Anyhow, you're right that we've never done that before.
Creating a synthetic event for the overall websocket is perfect. Nailed that. :D
I think there may be another direction to get the websocket events drawn on top of the synthetic one. Slightly less invasive.
I did a quick spike to explore the idea and curious for your thoughts. This a diff on top of your CL: https://gist.github.com/paulirish/9542af86037e08ec513e684be11f34b1
I am hoping you can tell me if this direction makes the forceDrawableLevel() method moot. I'm still unsure of what behavior that check is fixing.
I'll add that a bunch of your (and my) edits to TimelineFlameChartNetworkDataProvider.ts are because the real websocket trace events don't have args like SynNetworkRequest. We could create Syn events for each of those websocket events that do have a mimetype/url/priority/etc... But.. I agree with your impl that it feels kinda unnecessary. I could go either way, though.
Regardless, you had a tough task here, as the infrastructure is not built well to suit this use case. Appreciate you giving it an honest go. :)
if (Types.TraceEvents.isTraceEventWebSocketCreate(event) || Types.TraceEvents.isTraceEventWebSocketInfo(event) ||let's move this implementation into NetworkRequestHandler.
Conceptually it sorta belongs. (no strong feelings.)
But, more importantly, the main thing I noticed... Right now when we do the `this.#events.push` in networkdataprovider... the events end up unsorted. And appendEventsAtLevel needs sorted. Otherwise the level placement is incorrect and inefficient.. something i noticed with a trace.
The existing `traceEngineData.NetworkRequests.byTime` already takes care of the sort. So.. moving into there would address that bug. :)
ph: mainEvent.ph,use Phase.COMPLETE, as this synth event has duration.
wsConnectionOpened: 'WebSocket connection opened',is the word `connection` needed on these strings? I don't care strongly but i'm happy to reduce words if possible :)
context.strokeStyle = '#ccc';Probably want something with `ThemeSupport.ThemeSupport.instance().getComputedValue` so that it changes in dark mode.
context.moveTo(start, lineY);
context.setLineDash([6, 4]);
context.lineTo(end, lineY);how about
```suggestion
context.setLineDash([3, 2]);
context.moveTo(start, lineY - 1);
context.lineTo(end, lineY - 1);
context.moveTo(start, lineY + 1);
context.lineTo(end, lineY + 1);
```
going double gives me more of a 'socket' feeling. and i think it needs the tighter spacing to help allow the tiny instant events to stand out.
Protocol.Network.ResourcePriority.Low :
event.args.data.priority;might as well set the args.data.priority to low in the synthetic event. can then avoid this special case.
Related details
- Christy Chen
- Jack Franklin
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Irish (Gerrit)
Paul Irish added 1 comment![]( "Open in Gerrit")
First up, I realize I basically handed you an impossible task.
"Let's have the instant events be part of a larger websocket event!"
As if that's trivial. :p Anyhow, you're right that we've never done that before.Creating a synthetic event for the overall websocket is perfect. Nailed that. :D
I think there may be another direction to get the websocket events drawn on top of the synthetic one. Slightly less invasive.
I did a quick spike to explore the idea and curious for your thoughts. This a diff on top of your CL: https://gist.github.com/paulirish/9542af86037e08ec513e684be11f34b1I am hoping you can tell me if this direction makes the forceDrawableLevel() method moot. I'm still unsure of what behavior that check is fixing.
I'll add that a bunch of your (and my) edits to TimelineFlameChartNetworkDataProvider.ts are because the real websocket trace events don't have args like SynNetworkRequest. We could create Syn events for each of those websocket events that do have a mimetype/url/priority/etc... But.. I agree with your impl that it feels kinda unnecessary. I could go either way, though.
Regardless, you had a tough task here, as the infrastructure is not built well to suit this use case. Appreciate you giving it an honest go. :)
Slightly less invasive.
I did a quick spike to explore the idea and curious for your thoughts.
In retrospect, I think much of the improvement in my attempt is worthwhile effects of merging WebsocketsHandler into NetworkRequestshandler.
So, feel free to gently ignore the details of that patch if it's annoying :)
Christy Chen (Gerrit)
Christy Chen added 1 comment![]( "Open in Gerrit")
First up, I realize I basically handed you an impossible task.
"Let's have the instant events be part of a larger websocket event!"
As if that's trivial. :p Anyhow, you're right that we've never done that before.Creating a synthetic event for the overall websocket is perfect. Nailed that. :D
I think there may be another direction to get the websocket events drawn on top of the synthetic one. Slightly less invasive.
I did a quick spike to explore the idea and curious for your thoughts. This a diff on top of your CL: https://gist.github.com/paulirish/9542af86037e08ec513e684be11f34b1I am hoping you can tell me if this direction makes the forceDrawableLevel() method moot. I'm still unsure of what behavior that check is fixing.
I'll add that a bunch of your (and my) edits to TimelineFlameChartNetworkDataProvider.ts are because the real websocket trace events don't have args like SynNetworkRequest. We could create Syn events for each of those websocket events that do have a mimetype/url/priority/etc... But.. I agree with your impl that it feels kinda unnecessary. I could go either way, though.
Regardless, you had a tough task here, as the infrastructure is not built well to suit this use case. Appreciate you giving it an honest go. :)
Thank you so much for the inputs!
I like the direction you are going in the refactor diff, though I was having a hard time making that work today... Typescript just wasnt happy about what I was trying to do here 😂, I'll spend more time on Monday.
I think your approach can remove the forceDrawableLevel() hack I put in, which was needed because the flamechart assumes there's no stacking of the events (which make sense :), so it wont draw the synth event when zoomed in without that check.
Anyways, I'll try and see if I can incorporate as much as your feedback and make the change less invasive.
Related details
- Jack Franklin
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christy Chen (Gerrit)
Christy Chen added 8 comments![]( "Open in Gerrit")
Christy ChenFirst up, I realize I basically handed you an impossible task.
"Let's have the instant events be part of a larger websocket event!"
As if that's trivial. :p Anyhow, you're right that we've never done that before.Creating a synthetic event for the overall websocket is perfect. Nailed that. :D
I think there may be another direction to get the websocket events drawn on top of the synthetic one. Slightly less invasive.
I did a quick spike to explore the idea and curious for your thoughts. This a diff on top of your CL: https://gist.github.com/paulirish/9542af86037e08ec513e684be11f34b1I am hoping you can tell me if this direction makes the forceDrawableLevel() method moot. I'm still unsure of what behavior that check is fixing.
I'll add that a bunch of your (and my) edits to TimelineFlameChartNetworkDataProvider.ts are because the real websocket trace events don't have args like SynNetworkRequest. We could create Syn events for each of those websocket events that do have a mimetype/url/priority/etc... But.. I agree with your impl that it feels kinda unnecessary. I could go either way, though.
Regardless, you had a tough task here, as the infrastructure is not built well to suit this use case. Appreciate you giving it an honest go. :)
Thank you so much for the inputs!
I like the direction you are going in the refactor diff, though I was having a hard time making that work today... Typescript just wasnt happy about what I was trying to do here 😂, I'll spend more time on Monday.
I think your approach can remove the forceDrawableLevel() hack I put in, which was needed because the flamechart assumes there's no stacking of the events (which make sense :), so it wont draw the synth event when zoomed in without that check.Anyways, I'll try and see if I can incorporate as much as your feedback and make the change less invasive.
I removed the websocket handler and just handle the events in Network request handler. I'm however having a hard time to merge all the events completely... need to do a lot of typing hack to make typescript happy because websocket events dont have necessary args most of the time :/
I pushed an update, which handles in the websocket event in network request handler but keep the websocket as a standalone event group. I'd love to hear your thoughts about this :)
pushed some updates, appreciate your feedback!
if (Types.TraceEvents.isTraceEventWebSocketCreate(event) || Types.TraceEvents.isTraceEventWebSocketInfo(event) ||let's move this implementation into NetworkRequestHandler.
Conceptually it sorta belongs. (no strong feelings.)
But, more importantly, the main thing I noticed... Right now when we do the `this.#events.push` in networkdataprovider... the events end up unsorted. And appendEventsAtLevel needs sorted. Otherwise the level placement is incorrect and inefficient.. something i noticed with a trace.
The existing `traceEngineData.NetworkRequests.byTime` already takes care of the sort. So.. moving into there would address that bug. :)
Not sure if I understand the bug. Currently when we create this.#events in the networkDataProvider, we push the WebSocket events first, which will occupied the row, and then we push traceEngineData.NetworkRequests.byTime, so the regular network requests are still sorted and level calculation would still work. Let me know if Im missing something!
use Phase.COMPLETE, as this synth event has duration.
Done
is the word `connection` needed on these strings? I don't care strongly but i'm happy to reduce words if possible :)
Done
Probably want something with `ThemeSupport.ThemeSupport.instance().getComputedValue` so that it changes in dark mode.
Got it. I picked something randomly. Open to suggestions.
context.moveTo(start, lineY);
context.setLineDash([6, 4]);
context.lineTo(end, lineY);how about
```suggestion
context.setLineDash([3, 2]);
context.moveTo(start, lineY - 1);
context.lineTo(end, lineY - 1);
context.moveTo(start, lineY + 1);
context.lineTo(end, lineY + 1);
```going double gives me more of a 'socket' feeling. and i think it needs the tighter spacing to help allow the tiny instant events to stand out.
Good suggestion, updated!
Protocol.Network.ResourcePriority.Low :
event.args.data.priority;might as well set the args.data.priority to low in the synthetic event. can then avoid this special case.
Did that, but this is taking care of the non-synth events so still needed.
Related details
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 6 comments![]( "Open in Gerrit")
hope you don't mind me jumping in - Paul asked me to take a look 😊
One other thought - we might have some parts of the codebase where we assume that if an event's type is not SyntheticNetworkRequest, then it is not on the network track. I think some of our recent work around overlays will be affected by this, and in particular the selected entry overlay in the performance panel. Can you make sure that this works OK and they are positioned accurately when you select a web socket event? This changed very recently (start of this week) so make sure you rebase.
If we define a type of `NetworkTrackEntry` which is the union of the synthetic network request and the websocket entry(s), that would avoid the duplication and long union types repeated in the network appender, and also mean we could land an `isNetworkTrackEntry()` method which would be handy.
export type SyntheticCompleteEventType = SyntheticWebSocketConnectionEvent;why do we need this vs just exposing the interface which we already do?
if (TraceEngine.Types.TraceEvents.isWebSocketTraceEvent(event) ||
TraceEngine.Types.TraceEvents.isSyntheticWebSocketConnectionEvent(event)) {this check is confusing me: the type def says the event is either `TraceEventData|SyntheticWebSocketConnectionEvent`, so why do we check for `isWebSocketTraceEvent` and `isSyntheticWebSocketConnectionEvent` ?
events: (TraceEngine.Types.TraceEvents.SyntheticNetworkRequest|TraceEngine.Types.TraceEvents.WebSocketEvent)[],would probably define `type NetworkTrackEvent = this union` to avoid repeating this a bunch
return this.decorateSyntheticWebSocketConnectionEvent(nit: can this be a JS private method please? (same with `decorateNetworkRequest` too)
if (entryOffsetRight <= this.chartViewport.windowLeftTime() &&
!(this.dataProvider.forceDrawableLevel && this.dataProvider.forceDrawableLevel(level))) {took me a minute to understand this conditional
how do you feel about:
```
const levelForcedDrawable = Boolean(this.dataProvider.forceDrawableLevel?.(level));
if(entryOffsetRight <= this.chartViewport.windowLeftTime && !levelForcedDrawable) {}
```
(or similar)
Related details
- Christy Chen
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 6 comments![]( "Open in Gerrit")
We also need a test for NetworkRequestsHandler please that loads in a trace with a web socket and asserts that we parse out the expected events.
});I am finding this really hard to understand...
so we go through each web socket we found, and create a synthetic event, and shift it onto the beginning, so that each entry in `webSocketData` will look roughly like:
```
// other fields
events: [syntheticEvent, the rest of the events...]
```
is that correct?
again, a comment or two might be useful here to try to break this down.
or potentially a nicer structure would be either making the synthetic event contain all the others (but I can see why that might not be ideal), or having:
```
{
syntheticEvent: event,
childEvents: [events],
}
```
or similar
webSocketTraceData: [...webSocketData.values()],just `webSockets` would be fine for the key here
function createSyntheticWebSocketConnectionEvent(could we add some documentation (jsdoc is fine) to this file to explain how we use all the various web socket events?
from my understanding, it seems that the synthetic event represents the entire duration of the connection, and then you can use its identifier to find all the events that happened during its lifecycle. is that accurate?
Also, if we put into `args.data` the set of all "child" events, could we change the data we export from this handler into being:
```
sockets: SyntheticWebSocketConnection[]
```
and then have less complexity elsewhere? wdyt?
if (this.webSocketIdToLevel.has(webSocketIdentifier)) {
level = this.webSocketIdToLevel.get(webSocketIdentifier) || 0;
} else {
level = getEventLevel(event, lastTimeByLevel);
this.webSocketIdToLevel.set(webSocketIdentifier, level);
}given that this method is about filtering an existing timeline, would we actually ever hit the `else` case here where we don't have a level set for a given web socket identifier?
forceDrawableLevel(levelIndex: number): boolean {
return this.#networkTrackAppender?.webSocketIdToLevel.has(levelIndex) || false;
}sorry, probably a silly question for my understanding, why do we need this? if the socket is in the view, it will be drawn anyway? what scenario does this fix/avoid?
Christy Chen (Gerrit)
Christy Chen added 12 comments![]( "Open in Gerrit")
We also need a test for NetworkRequestsHandler please that loads in a trace with a web socket and asserts that we parse out the expected events.
Will do that tomorrow!
hope you don't mind me jumping in - Paul asked me to take a look 😊
One other thought - we might have some parts of the codebase where we assume that if an event's type is not SyntheticNetworkRequest, then it is not on the network track. I think some of our recent work around overlays will be affected by this, and in particular the selected entry overlay in the performance panel. Can you make sure that this works OK and they are positioned accurately when you select a web socket event? This changed very recently (start of this week) so make sure you rebase.
If we define a type of `NetworkTrackEntry` which is the union of the synthetic network request and the websocket entry(s), that would avoid the duplication and long union types repeated in the network appender, and also mean we could land an `isNetworkTrackEntry()` method which would be handy.
Not at all! Thank you so much for reviewing 😊
I just rebased and so far I'm not seeing any issue, but could you provide more details about the scenarios that I should check related to the overlay? Any documentation or steps I need to follow?
Pushed an update. Will work on a few pending tasks tomorrow.
I am finding this really hard to understand...
so we go through each web socket we found, and create a synthetic event, and shift it onto the beginning, so that each entry in `webSocketData` will look roughly like:
```
// other fields
events: [syntheticEvent, the rest of the events...]
```is that correct?
again, a comment or two might be useful here to try to break this down.
or potentially a nicer structure would be either making the synthetic event contain all the others (but I can see why that might not be ideal), or having:
```
{
syntheticEvent: event,
childEvents: [events],
}
```or similar
That was actually my initial implementation, but then I hit some issues because the flameChart assumes "flat events" and the nested events would need to be handled differently. But I will give this approach another try tomorrow.
just `webSockets` would be fine for the key here
Done
export type SyntheticCompleteEventType = SyntheticWebSocketConnectionEvent;why do we need this vs just exposing the interface which we already do?
You are right, this is extra. Removed
if (TraceEngine.Types.TraceEvents.isWebSocketTraceEvent(event) ||
TraceEngine.Types.TraceEvents.isSyntheticWebSocketConnectionEvent(event)) {this check is confusing me: the type def says the event is either `TraceEventData|SyntheticWebSocketConnectionEvent`, so why do we check for `isWebSocketTraceEvent` and `isSyntheticWebSocketConnectionEvent` ?
Ah, the type change in the param is not necessary. updated
events: (TraceEngine.Types.TraceEvents.SyntheticNetworkRequest|TraceEngine.Types.TraceEvents.WebSocketEvent)[],would probably define `type NetworkTrackEvent = this union` to avoid repeating this a bunch
Done
if (this.webSocketIdToLevel.has(webSocketIdentifier)) {
level = this.webSocketIdToLevel.get(webSocketIdentifier) || 0;
} else {
level = getEventLevel(event, lastTimeByLevel);
this.webSocketIdToLevel.set(webSocketIdentifier, level);
}given that this method is about filtering an existing timeline, would we actually ever hit the `else` case here where we don't have a level set for a given web socket identifier?
We are recalculating all level of events when the filtering happens, so the map will also get reset and recalculate too.
return this.decorateSyntheticWebSocketConnectionEvent(nit: can this be a JS private method please? (same with `decorateNetworkRequest` too)
Done
forceDrawableLevel(levelIndex: number): boolean {
return this.#networkTrackAppender?.webSocketIdToLevel.has(levelIndex) || false;
}sorry, probably a silly question for my understanding, why do we need this? if the socket is in the view, it will be drawn anyway? what scenario does this fix/avoid?
In the FlameChart.ts, when filtering through the events for a level, it starts from the last event of that level and stops when it hit an event that has start time greater than the filtering window.
For example, in this websocket level we have A(socket event), B, C, D. If C event has start time greater than the window, the rest of the events (A and B) wont be drawn.
So if this level is the force Drawable level, we wont stop at event C and will include the socket event A.
I'll add some comments to explain this in the code :)
if (entryOffsetRight <= this.chartViewport.windowLeftTime() &&
!(this.dataProvider.forceDrawableLevel && this.dataProvider.forceDrawableLevel(level))) {took me a minute to understand this conditional
how do you feel about:
```
const levelForcedDrawable = Boolean(this.dataProvider.forceDrawableLevel?.(level));
if(entryOffsetRight <= this.chartViewport.windowLeftTime && !levelForcedDrawable) {}
```(or similar)
- Jack Franklin
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christy Chen (Gerrit)
Christy Chen added 5 comments![]( "Open in Gerrit")
Christy ChenWe also need a test for NetworkRequestsHandler please that loads in a trace with a web socket and asserts that we parse out the expected events.
Will do that tomorrow!
Done
Christy Chenhope you don't mind me jumping in - Paul asked me to take a look 😊
One other thought - we might have some parts of the codebase where we assume that if an event's type is not SyntheticNetworkRequest, then it is not on the network track. I think some of our recent work around overlays will be affected by this, and in particular the selected entry overlay in the performance panel. Can you make sure that this works OK and they are positioned accurately when you select a web socket event? This changed very recently (start of this week) so make sure you rebase.
If we define a type of `NetworkTrackEntry` which is the union of the synthetic network request and the websocket entry(s), that would avoid the duplication and long union types repeated in the network appender, and also mean we could land an `isNetworkTrackEntry()` method which would be handy.
Not at all! Thank you so much for reviewing 😊
I just rebased and so far I'm not seeing any issue, but could you provide more details about the scenarios that I should check related to the overlay? Any documentation or steps I need to follow?
Oh interesting, yes there are some issues when selecting the events. Fixed now.
I have addressed most of the feedback! Appreciate if you can take a look again. Thank! 😊
Christy ChenI am finding this really hard to understand...
so we go through each web socket we found, and create a synthetic event, and shift it onto the beginning, so that each entry in `webSocketData` will look roughly like:
```
// other fields
events: [syntheticEvent, the rest of the events...]
```is that correct?
again, a comment or two might be useful here to try to break this down.
or potentially a nicer structure would be either making the synthetic event contain all the others (but I can see why that might not be ideal), or having:
```
{
syntheticEvent: event,
childEvents: [events],
}
```or similar
That was actually my initial implementation, but then I hit some issues because the flameChart assumes "flat events" and the nested events would need to be handled differently. But I will give this approach another try tomorrow.
Updated, I think it's cleaner now.
could we add some documentation (jsdoc is fine) to this file to explain how we use all the various web socket events?
from my understanding, it seems that the synthetic event represents the entire duration of the connection, and then you can use its identifier to find all the events that happened during its lifecycle. is that accurate?
Also, if we put into `args.data` the set of all "child" events, could we change the data we export from this handler into being:
```
sockets: SyntheticWebSocketConnection[]
```and then have less complexity elsewhere? wdyt?
Updated. it's now exporting WebSocketTraceData[] and each WebSocketTraceData has
{
events: the list of WebSocket events
syntheticConnectionEvent: the synthetic event representing the entire WebSocket connection
}
Related details
- Jack Franklin
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christy Chen (Gerrit)
Christy Chen added 1 comment![]( "Open in Gerrit")
Friendly ping 😊 appreciate if you can have a look!
Related details
- Jack Franklin
- Paul Irish
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |