feat: Add time information to Call and Network tabs in Trace Viewer #33935
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Tagging #33823 |
| @@ -94,6 +99,10 @@ const RequestTab: React.FunctionComponent<{ | |||
| {resource.request.queryString.map(param => `${param.name}: ${param.value}`).join('\n')} | |||
| </div> | |||
| </> : null} | |||
| <div className='network-request-details-header'>Time</div> | |||
There was a problem hiding this comment.
What's the point in showing these values in the details, given that they are already present in the network table where one can see other network requests and compare their start time/duration?
There was a problem hiding this comment.
It's a strange omission for it to not be available. Why would expanding the details of something show fewer details?
While you're right that they can only compare when the details are not open, I think it's still valuable to be able to view all the metrics on this screen.
There was a problem hiding this comment.
While I agree with you that it's nice to see this information in the details view, I still cannot come up with a good explanation why it should be shown above request headers. Maybe push it below headers at least? We should probably put timing information into its own tab and populate it with other details that we have, but I don't remember anyone asking for this.
| @@ -94,6 +99,10 @@ const RequestTab: React.FunctionComponent<{ | |||
| {resource.request.queryString.map(param => `${param.name}: ${param.value}`).join('\n')} | |||
| </div> | |||
| </> : null} | |||
| <div className='network-request-details-header'>Time</div> | |||
There was a problem hiding this comment.
While I agree with you that it's nice to see this information in the details view, I still cannot come up with a good explanation why it should be shown above request headers. Maybe push it below headers at least? We should probably put timing information into its own tab and populate it with other details that we have, but I don't remember anyone asking for this.
| @@ -176,7 +176,7 @@ export const Workbench: React.FunctionComponent<{ | |||
| const callTab: TabbedPaneTabModel = { | |||
| id: 'call', | |||
| title: 'Call', | |||
| render: () => <CallTab action={activeAction} sdkLanguage={sdkLanguage} /> | |||
| render: () => <CallTab action={activeAction} executionStartTime={model?.startTime ?? 0} executionStartWallTime={model?.wallTime ?? Date.now()} sdkLanguage={sdkLanguage} /> | |||
There was a problem hiding this comment.
It should never happen that model?.wallTime is undefined when activeAction is not null, why pass Date.now() rather than 0 or -1? Would be cleaner if CallTab accepted single parameter { action, startTime, startWallTime } | undefined, can we do that?
There was a problem hiding this comment.
Doing so requires changing the component significantly so that we properly represent model and activeAction as being intertwined state. Currently TypeScript doesn't know they're related.
I would prefer to not change this and leave the coalescence placeholders, but I will change this if you so desire. I personally think this would be overly specific type handling, though it's obviously more correct.
| { | ||
| <> | ||
| <div className='call-section'>Time</div> | ||
| {wallTime && <DateTimeCallLine name='wall time:' value={wallTime}/>} |
There was a problem hiding this comment.
I don't think wallTime is ever null at this line.
There was a problem hiding this comment.
This is why we need that lint rule :P
…tartTime-ui-mode-fixes
Test results for "tests 1"6 flaky37337 passed, 650 skipped Merge workflow run. |
|
What is Start time? |
| @@ -176,7 +176,7 @@ export const Workbench: React.FunctionComponent<{ | |||
| const callTab: TabbedPaneTabModel = { | |||
| id: 'call', | |||
| title: 'Call', | |||
| render: () => <CallTab action={activeAction} sdkLanguage={sdkLanguage} /> | |||
| render: () => <CallTab action={activeAction} executionStartTime={model?.startTime ?? 0} executionStartWallTime={model?.wallTime ?? 0} sdkLanguage={sdkLanguage} /> | |||
There was a problem hiding this comment.
I don't think "execution" helps here, start time usually applies to execution, maybe name it timeOrigin?
| const startTimeMillis = action.startTime - executionStartTime; | ||
| const startTime = msToString(startTimeMillis); | ||
|
|
||
| const wallTimeMillis = startTimeMillis + executionStartWallTime; |
There was a problem hiding this comment.
This would be just a guess at this point as we can't add ticks to millis, do we really want to explicitly mislead? I would omit.
There was a problem hiding this comment.
Offline discussion with Yury where I am saying that we should not do wall time math: maybe we could formalize model.startTime and then we can remove the wallTime from individual actions and operate in terms or ticks (monotonic time) for actions only.
Adds
startTimedisplay to the Call tab.Adds various time metric display to the Network tab expanded view.
Minor code cleanup to make it more Reacty.