Add routes for time-to-first-byte stats #281
Conversation
stats/lib/handler.js
Outdated
| @@ -14,7 +14,9 @@ import { | |||
| fetchRetrievalSuccessRate, | |||
| fetchDealSummary, | |||
| fetchDailyRetrievalResultCodes, | |||
| fetchDailyMinerRSRSummary | |||
| fetchDailyMinerRSRSummary, | |||
| fetchDailyRetrievalTimes, | |||
There was a problem hiding this comment.
"Retrieval times" isn't self explanatory - it could for example also be the timestamps at which retrievals have been attempted. What about this?
| fetchDailyRetrievalTimes, | |
| fetchDailyRetrievalTimings, |
There was a problem hiding this comment.
See for example chrome dev tools also using "timings":
There was a problem hiding this comment.
Thanks for suggestion. I have renamed everything from times to timings including table, routes and functions.
stats/lib/stats-fetchers.js
Outdated
| const { rows } = await pgPools.evaluate.query(` | ||
| SELECT | ||
| day::text, | ||
| percentile_cont(0.5) WITHIN GROUP (ORDER BY time_to_first_byte_p50) AS ttfb_p50 |
There was a problem hiding this comment.
Having variables time_to_first_byte_p50 and ttfb_p50 is confusing, as the names don't tell us how they differ. Can you think of a more semantic name for the computed column?
There was a problem hiding this comment.
There are two aspects to consider:
- What name do we want to use in the SQL query?
- What property name do we want to use in the response body?
Also, note that a p50 value calculated from a list of p50 values is not a true global p50 value; it's just an approximation we decided to use to keep this initial implementation simple.
In that light, I propose to use the name ttfb in the response.
[
{ "day": "2025-01-08", "ttfb": 850 }
{ "day": "2025-01-09", "ttfb": 930 }
]I think this name works well for the SQL query, too, but I don't mind using a different name there.
There was a problem hiding this comment.
One more idea to consider - should we encode the unit (milliseconds) in the property name?
[
{ "day": "2025-01-08", "ttfb_ms": 850 }
{ "day": "2025-01-09", "ttfb_ms": 930 }
]There was a problem hiding this comment.
I wasn't sure if I should use abbreviation inside the table or not but have renamed time_to_first_byte_p50 column to ttfb_p50.
Computed p50 response value is now also named ttfb_ms as suggested by Miro. Thank you for input 🙏🏻
stats/lib/stats-fetchers.js
Outdated
| const { rows } = await pgPools.evaluate.query(` | ||
| SELECT | ||
| day::text, | ||
| percentile_cont(0.5) WITHIN GROUP (ORDER BY time_to_first_byte_p50) AS ttfb_p50 |
There was a problem hiding this comment.
percentile_cont returns a double precision floating number, e.g. 20.124. This is likely to create a sense of false precision.
Let's convert the number to an integer, e.g. using the Postgres CEIL() function.
- We could also use
ROUND()(10.2→10, 10.8→11), but I prefer to be conservative in what we produce and prefer to report slightly worse TTFB values rather than slightly better values.
This is a minor detail, though - I am fine with any rounding function.
What's important to me is to produce integer values (number of milliseconds), since that's the precision we are getting from the checker ndoes.
There was a problem hiding this comment.
I have missed this as I was testing using round int values. I have added rounding to queries and we're now also rounding computed p50 values before inserting them to the database (in both cases we're rounding using ceil). I am going to change tests to include more odd values.
pyropy
force-pushed
the
add/ttfb-stats
branch
from
d15f62e to
067e7d4
Compare
|
I am marking this ready for review, although CI will fail before filecoin-station/spark-evaluate#434 gets merged. |
|
Related follow-up issue #288 |
Adds routes for fetching global and per miner time to first byte stats.
Related to space-meridian/roadmap#208