Request metrics improvements #8420
Conversation
Update groupLabel to use a type definition Include effective Latency and Average Speed in the metrics Include parallel and sequential request counts
| } | ||
|
|
||
| const urlData = Object.entries(groupData.urls); | ||
| const sumLatency = urlData.reduce((result, url) => (result + (url[1].metrics?.latency ?? 0)), 0); |
There was a problem hiding this comment.
Should we discard 0 latency from equation, ideally it should never happen though right?
There was a problem hiding this comment.
it should never happen really, the 0 here is forced to be type safe.
| const effectiveParallelLatency = Math.max(...parallelLatencies, 0); | ||
| const effectiveSequentialLatency = sequentialLatencies.reduce((sum, latency) => sum + latency, 0); | ||
|
|
||
| const effectiveLatency = effectiveParallelLatency + effectiveSequentialLatency; |
There was a problem hiding this comment.
Do these need to be summed or should we get the avg especially for sequential latency?
There was a problem hiding this comment.
as far as I'm aware, for parallel requests the latency is the maximum latency of all requests and then sequential requests do add up to get the effective latency as if you execute two sequential requests and one takes 3s and the other one 5s then you waited a total of 8s.
that is the reason why I added the avg latency overall.
| const averageSpeedMbps = averageSpeedBps / 1_000_000; // Convert to Mbps | ||
|
|
||
| return { | ||
| averageSpeedMbps, |
There was a problem hiding this comment.
I guess we can use avg speed to decide whether download files or whatsoever.
| performance.mark('tti', options); | ||
| } | ||
|
|
||
| public measureTimeToInteraction() { |
There was a problem hiding this comment.
Yay, long anticipated metric is here :)
There was a problem hiding this comment.
yes, but is nothing more than an output to the console at the moment.. I need to figure out where and how to include it.
There was a problem hiding this comment.
Should I create another metric on the server to collect this?
There was a problem hiding this comment.
@rahimrahman can it be optional? I think that'll be very nice to have but haven't checked into how to include it as is a totally different metric collected in a different way and somewhere else in the code
| categorizeRequests(groupLabel: RequestGroupLabel): CategorizedRequests { | ||
| const parallel: ClientResponseMetrics[] = []; | ||
| const sequential: ClientResponseMetrics[] = []; | ||
| const groupData = this.requestGroups.get(groupLabel); | ||
| if (!groupData) { | ||
| return {parallel, sequential}; | ||
| } | ||
|
|
||
| // Sort requests by start time | ||
| const requestsMetrics = Object.entries(groupData.urls).map((e) => e[1].metrics).filter((m) => m != null); | ||
| requestsMetrics.sort((a, b) => a.startTime - b.startTime); | ||
|
|
||
| let lastEndTime = 0; | ||
|
|
||
| for (const metrics of requestsMetrics) { | ||
| if (metrics.startTime < lastEndTime) { | ||
| // Overlapping request -> Parallel | ||
| parallel.push(metrics); | ||
| } else { | ||
| // Non-overlapping request -> Sequential | ||
| sequential.push(metrics); | ||
| } | ||
|
|
||
| // Update the last end time | ||
| lastEndTime = Math.max(lastEndTime, metrics.endTime); | ||
| } | ||
|
|
||
| return {parallel, sequential}; | ||
| } | ||
|
|
||
| calculateAverageSpeedWithCategories = ( | ||
| categorizedRequests: CategorizedRequests, | ||
| elapsedTimeInSeconds: number, // Observed total elapsed time in seconds | ||
| ): { averageSpeedMbps: number; effectiveLatency: number } => { | ||
| const {parallel, sequential} = categorizedRequests; | ||
|
|
||
| // Step 1: Calculate total data size in bits | ||
| const totalDataBits = [...parallel, ...sequential].reduce((sum, req) => sum + (req.compressedSize * 8), 0); | ||
|
|
||
| // Step 2: Calculate effective latency | ||
| const parallelLatencies = parallel.map((req) => req.latency); | ||
| const sequentialLatencies = sequential.map((req) => req.latency); | ||
|
|
||
| const effectiveParallelLatency = Math.max(...parallelLatencies, 0); | ||
| const effectiveSequentialLatency = sequentialLatencies.reduce((sum, latency) => sum + latency, 0); | ||
|
|
||
| const effectiveLatency = effectiveParallelLatency + effectiveSequentialLatency; | ||
|
|
||
| // Step 3: Calculate data transfer time | ||
| const dataTransferTime = elapsedTimeInSeconds - (effectiveLatency / 1000); | ||
|
|
||
| // Handle edge case: if data transfer time is zero or negative | ||
| if (dataTransferTime <= 0) { | ||
| return {averageSpeedMbps: 0, effectiveLatency: 0}; | ||
| } | ||
|
|
||
| // Step 4: Calculate average speed | ||
| const averageSpeedBps = totalDataBits / dataTransferTime; // Speed in bps | ||
| const averageSpeedMbps = averageSpeedBps / 1_000_000; // Convert to Mbps | ||
|
|
||
| return { | ||
| averageSpeedMbps, | ||
| effectiveLatency, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
I've made a significant proposed improvement based on data observation. I will try to explain as much as possible in this PR: #8439.
Summary
More improvements on network metrics include speed, effective latency and more importantly some changes with the entry fetching endpoints.
Ticket Link
https://mattermost.atlassian.net/browse/MM-61878
Release Note