fix: make each Insights metric measure what it claims - #374
Merged
Merged
Conversation
Signed-off-by: Priyanshu-u07 <connect.priyanshu8271@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The tile read "Avg Latency (TTFT)" and showed neither.
coalesce(ttft_ms, latency_ms)gave time to first token for streaming requests and total duration for the rest, averaged together, so the number moved with the streaming share of traffic rather than with performance. More streaming made latency appear to improve.Each expression now means its name.
latency_msis total duration,ttft_msis averaged only over the rows that have one, and the throughput figures use duration rather than the inverse coalesce they had.Both averages carry
samples, the count of rows behind them. The issue asks for TTFT "alongside its coverage": without it, an average over two streaming requests in a hundred reads the same as one over a hundred.The dashboard is in this PR too
Seven places claimed TTFT while showing something else. Fixing only the backend would have left the tile labelled TTFT while displaying pure latency, a larger number, which reads as a sudden regression.
Relabelled: the metric tile, the chart subtitle, the legend and series names, and two table headers. The tile subtitle now carries real TTFT with its coverage, and falls back to req/min when nothing streamed.
Two more coalesces lived in the frontend and are gone. The logs table showed
ttft_ms ?? latency_msunder a "Latency" header, and the CSV export wrote the same into a column headed "Latency (ms)".Two details the shape depends on
avg_ttft_mson a timeseries bucket is null, not zero, when nothing streamed in that bucket. Zero would claim a first token arrived instantly, and anything averaging these would be diluted by every bucket with nostreaming traffic — this issue one layer up.
avg_latency_msstays non-nullable: every returned bucket has at least one request and so always has a latency.ttft_msis optional-chained in the tile. It is a new field, so a response from an older backend has none, and an unguarded access would take the tile down during a rolling deploy.Closes #304