-
Notifications
You must be signed in to change notification settings - Fork 42
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
fix: associate network info with each step #154
fix: associate network info with each step #154
Conversation
@@ -78,6 +79,11 @@ export class PluginManager { | |||
return this.plugins.get(Type.name) as T; | |||
} | |||
|
|||
onStep(step: Step) { | |||
this.get(BrowserConsole) && (this.get(BrowserConsole)._currentStep = step); |
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.
Will replace this with EventEmitter at some point if we require more integration between runner and plugins.
url, | ||
request, | ||
type, | ||
method, | ||
start: timestamp, | ||
requestSentTime: timestamp, |
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 change was done after some investigation with the chrome network events and how we will use it in the waterfall. copy pasting from internal gist
So here is the gist that i found and we need to change
- we will use requestTime for the waterfall stuff
- start/end represents the timestamp of when request is sent and when request completed, so we can calculate the queue time as follows
const queuedTime = payload.response.timing.request_time - payload.response.start
if (queuedTime > 0) {
// convert in ms and use it in waterfall
}
- we discard end timing for now as its no useful. Also i think we can rename it to something like requestStartTime and responseEndTime to make it easier
@Kerry350 is aware of this issue.
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.
LGTM 👍
Thanks for making things easier here 🙏
requestSentTime: number; | ||
loadEndTime: 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.
Much clearer 👍
start/end
in favor ofrequestSentTime/loadEndTime
as its when events are written and not the actual duration./cc @Kerry350