Skip to content
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.

Sign up for GitHub

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

Add routes for time-to-first-byte stats #281

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add routes for time-to-first-byte stats #281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea10249
Add routes for time-to-first-byte stats
pyropy Dec 24, 2024
c4e3370
Make retrieval time stats more generic
pyropy Dec 24, 2024
d15f62e
Add tests for retrieval times
pyropy Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion stats/lib/handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import {
fetchRetrievalSuccessRate,
fetchDealSummary,
fetchDailyRetrievalResultCodes,
fetchDailyMinerRSRSummary
fetchDailyMinerRSRSummary,
fetchDailyRetrievalTimes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Retrieval times" isn't self explanatory - it could for example also be the timestamps at which retrievals have been attempted. What about this?

Suggested change
fetchDailyRetrievalTimes,
fetchDailyRetrievalTimings,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See for example chrome dev tools also using "timings":

Screenshot 2025-01-05 at 22 43 59

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to use timings

fetchDailyMinerRetrievalTimes
} from './stats-fetchers.js'

import { handlePlatformRoutes } from './platform-routes.js'
Expand Down Expand Up @@ -108,6 +110,10 @@ const handler = async (req, res, pgPools, SPARK_API_BASE_URL) => {
await respond(fetchMinersRSRSummary)
} else if (req.method === 'GET' && url === '/retrieval-result-codes/daily') {
await respond(fetchDailyRetrievalResultCodes)
} else if (req.method === 'GET' && url === '/retrieval-times/daily') {
await respond(fetchDailyRetrievalTimes)
} else if (req.method === 'GET' && segs[0] === 'miner' && segs[1] && segs[2] === 'retrieval-times' && segs[3] === 'summary') {
await respond(fetchDailyMinerRetrievalTimes, segs[1])
} else if (req.method === 'GET' && segs[0] === 'miner' && segs[1] && segs[2] === 'retrieval-success-rate' && segs[3] === 'summary') {
await respond(fetchDailyMinerRSRSummary, segs[1])
} else if (req.method === 'GET' && segs[0] === 'miner' && segs[1] && segs[2] === 'deals' && segs[3] === 'eligible' && segs[4] === 'summary') {
Expand Down
45 changes: 45 additions & 0 deletions stats/lib/stats-fetchers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,3 +270,48 @@ export const fetchDailyRetrievalResultCodes = async (pgPools, filter) => {
const stats = Object.entries(days).map(([day, rates]) => ({ day, rates }))
return stats
}

/**
* Fetches daily global retrieval time statistics
* @param {import('@filecoin-station/spark-stats-db').PgPools} pgPools
* @param {import('./typings.js').DateRangeFilter} filter
*/
export const fetchDailyRetrievalTimes = async (pgPools, filter) => {
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two aspects to consider:

  1. What name do we want to use in the SQL query?
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ttfb_ms" lgtm!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

FROM retrieval_times
WHERE day >= $1 AND day <= $2
GROUP BY day
ORDER BY day
`, [
filter.from,
filter.to
])
return rows
}

/**
* Fetches per miner daily retrieval time statistics
* @param {import('@filecoin-station/spark-stats-db').PgPools} pgPools
* @param {import('./typings.js').DateRangeFilter} filter
* @param {string} minerId
*/
export const fetchDailyMinerRetrievalTimes = async (pgPools, { from, to }, minerId) => {
const { rows } = await pgPools.evaluate.query(`
SELECT
day::text,
miner_id,
percentile_cont(0.5) WITHIN GROUP (ORDER BY time_to_first_byte_p50) AS ttfb_p50
FROM retrieval_times
WHERE miner_id = $1 AND day >= $2 AND day <= $3
GROUP BY day, miner_id
ORDER BY day
`, [
minerId,
from,
to
])
return rows
}
76 changes: 76 additions & 0 deletions stats/test/handler.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ describe('HTTP request handler', () => {
await pgPools.evaluate.query('DELETE FROM retrieval_stats')
await pgPools.evaluate.query('DELETE FROM daily_participants')
await pgPools.evaluate.query('DELETE FROM daily_deals')
await pgPools.evaluate.query('DELETE FROM retrieval_times')
await pgPools.stats.query('DELETE FROM daily_scheduled_rewards')
await pgPools.stats.query('DELETE FROM daily_reward_transfers')
await pgPools.stats.query('DELETE FROM daily_retrieval_result_codes')
Expand Down Expand Up @@ -708,6 +709,65 @@ describe('HTTP request handler', () => {
])
})
})

describe('miner retrieval time stats', () => {
beforeEach(async () => {
// before the range
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-09', minerId: 'f1one', taskId: 'cidone::f1one::0', timeToFirstByteP50: 1000 })
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-09', minerId: 'f1two', taskId: 'cidone::f1two::0', timeToFirstByteP50: 1000 })
// in the range
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-20', minerId: 'f1one', taskId: 'cidone::f1one::1', timeToFirstByteP50: 1000 })
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-20', minerId: 'f1two', taskId: 'cidone::f1two::1', timeToFirstByteP50: 1000 })

await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-10', minerId: 'f1one', taskId: 'cidone::f1one::2', timeToFirstByteP50: 3000 })
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-10', minerId: 'f1two', taskId: 'cidone::f1two::2', timeToFirstByteP50: 3000 })
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-10', minerId: 'f1one', taskId: 'cidone::f1one::3', timeToFirstByteP50: 1000 })
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-10', minerId: 'f1two', taskId: 'cidone::f1two::3', timeToFirstByteP50: 1000 })
// after the range
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-21', minerId: 'f1one', taskId: 'cidone::f1one::4', timeToFirstByteP50: 1000 })
await givenRetrievalTimes(pgPools.evaluate, { day: '2024-01-21', minerId: 'f1two', taskId: 'cidone::f1two::4', timeToFirstByteP50: 1000 })
})

it('lists daily retrieval times in given date range', async () => {
const res = await fetch(
new URL(
'/retrieval-times/daily?from=2024-01-10&to=2024-01-20',
baseUrl
), {
redirect: 'manual'
}
)
await assertResponseStatus(res, 200)

const stats = /** @type {{ day: string, success_rate: number }[]} */(
await res.json()
)
assert.deepStrictEqual(stats, [
{ day: '2024-01-10', ttfb_p50: 2000 },
{ day: '2024-01-20', ttfb_p50: 1000 }
])
})

it('lists daily retrieval times summary for specified miner in given date range', async () => {
const res = await fetch(
new URL(
'/miner/f1one/retrieval-times/summary?from=2024-01-10&to=2024-01-20',
baseUrl
), {
redirect: 'manual'
}
)
await assertResponseStatus(res, 200)

const stats = /** @type {{ day: string, success_rate: number }[]} */(
await res.json()
)
assert.deepStrictEqual(stats, [
{ day: '2024-01-10', miner_id: 'f1one', ttfb_p50: 2000 },
{ day: '2024-01-20', miner_id: 'f1one', ttfb_p50: 1000 }
])
})
})
})

/**
Expand Down Expand Up @@ -783,3 +843,19 @@ const givenDailyDealStats = async (pgPool, {
retrievable
])
}

/**
*
* @param {import('../lib/platform-stats-fetchers.js').Queryable} pgPool
* @param {object} data
* @param {string} data.day
* @param {string} data.minerId
* @param {string} data.taskId
* @param {number} data.timeToFirstByteP50
*/
const givenRetrievalTimes = async (pgPool, { day, minerId, taskId, timeToFirstByteP50 }) => {
await pgPool.query(
'INSERT INTO retrieval_times (day, miner_id, task_id, time_to_first_byte_p50) VALUES ($1, $2, $3, $4)',
[day, minerId ?? 'f1test', taskId ?? 'cidone::f1test::0', timeToFirstByteP50]
)
}
Loading