-
Notifications
You must be signed in to change notification settings - Fork 4
Use @fastify/fastify-postgres. CheckerNetwork/roadmap#220 #341
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your submission @Goddhi 🚀
Good start, but there's some changes that need to be addressed:
- Install
@fastify/postgres
package in correct workspace - Revert or rethink on how the migrations are done
- Avoid creating new
Fastify
instances in order to register databases - Figure out how to perform queries from fastify handlers
db/package.json
Outdated
"@fastify/postgres": "^5.2.0", | ||
"@fastify/url-data": "^6.0.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@fastify/postgres": "^5.2.0", | |
"@fastify/url-data": "^6.0.3" |
I think we should remove these packages from db
workspace and install @fastify/postgres
inside the stats
workspace only. You can install workspace specific packages by running npm install @fastify/postgres -w stats
stats/bin/migrate.js
Outdated
EVALUATE_DB_URL | ||
} = process.env | ||
|
||
const app = Fastify({ logger: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we shouldn't really create a new Fastify
instance in order to run migrations. This should be done without the @fastify/postgres
package.
stats/bin/spark-stats.js
Outdated
} = process.env | ||
|
||
const pgPools = await getPgPools() | ||
|
||
const dbFastify = Fastify({ logger: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't create another Fastify
instance here as we already do it inside createApp
function. Rather, we should move registering the @fastify/postgres
plugin and databases to be executed within createApp
funciton.
stats/bin/spark-stats.js
Outdated
|
||
await dbFastify.register(fastifyPostgres, { | ||
connectionString: DATABASE_URL, | ||
name: 'stats', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is wrong because because we're instantiate only connection to stats
database, but we do reference evaluate
database bellow.
stats/lib/db.js
Outdated
let fastifyApp = null | ||
let isInitialized = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let fastifyApp = null | |
let isInitialized = false |
I would avoid using globals in this case.
stats/lib/db.js
Outdated
if (isInitialized) return | ||
|
||
// Create a minimal Fastify instance for database connections | ||
fastifyApp = Fastify({ logger: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating a new Fastify
instance I think it would be better to pass down existing instance as function argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to what @pyropy wrote
@pyropy I do appreciate your reviews, i have made the following changes as requested, kindly look into thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to avoid having dead code around. Also, let's try to use new way of accessing databases in the API handlers.
stats/lib/app.js
Outdated
const pgPools = { | ||
stats: app.pg.stats, | ||
evaluate: app.pg.evaluate, | ||
async end() { | ||
await app.close() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const pgPools = { | |
stats: app.pg.stats, | |
evaluate: app.pg.evaluate, | |
async end() { | |
await app.close() | |
} | |
} |
I don't see this being used anywhere. If it's not used, let's removed. YAGNI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is being passed to addRoutes()
and addPlatformRoutes()
below. However, instead of passing pgPools
, the route handlers should access app.pg.stats
etc. directly
stats/bin/migrate.js
Outdated
const { | ||
DATABASE_URL, | ||
EVALUATE_DB_URL | ||
} = process.env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { | |
DATABASE_URL, | |
EVALUATE_DB_URL | |
} = process.env |
These constants seem not to be used anywhere. Let's delete them.
stats/bin/migrate.js
Outdated
console.log('Running migrations for stats database...') | ||
const pgPools = await getPgPools() | ||
|
||
// @ts-ignore - PgPoolStats actually does have a query method at runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to add correct types to migrate functions rather then using @ts-ignore
. Using it should be avoided.
stats/bin/migrate.js
Outdated
// @ts-ignore - PgPoolStats actually does have a query method at runtime | ||
await migrateStatsDB(pgPools.stats) | ||
|
||
// @ts-ignore - Similarly for evaluate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, let's please not use @ts-ignore
@Goddhi I suggest you link the issue you're working on in the PR description (and not in title). In this case you should've linked this issue CheckerNetwork/roadmap#220 |
stats/lib/app.js
Outdated
const pgPools = { | ||
stats: app.pg.stats, | ||
evaluate: app.pg.evaluate, | ||
async end() { | ||
await app.close() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is being passed to addRoutes()
and addPlatformRoutes()
below. However, instead of passing pgPools
, the route handlers should access app.pg.stats
etc. directly
@pyropy and @juliangruber have made commits that addresses the reviews. |
stats/lib/platform-routes.js
Outdated
const pgPools = adaptPgPools(request.server.pg); | ||
reply.send(await fetchTopEarningParticipants(pgPools.stats, request.filter)) }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const pgPools = adaptPgPools(request.server.pg); | |
reply.send(await fetchTopEarningParticipants(pgPools.stats, request.filter)) }) | |
reply.send(await fetchTopEarningParticipants(request.server.pg.stats, request.filter)) }) |
What do we need the adaptPgPools
function for here?
stats/lib/routes.js
Outdated
app.register(async app => { | ||
app.addHook('preHandler', filterPreHandlerHook) | ||
app.addHook('onSend', filterOnSendHook) | ||
|
||
app.get('/deals/daily', async (/** @type {RequestWithFilter} */ request, reply) => { | ||
const pgPools = adaptPgPools(request.server.pg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about instead changing the signature of fetchDaily...
etc to accept request.server.pg
? fetchDailyDealStats(request.server.pg, request.filter)
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, noted, i will do that now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on changing the signature and dropping the adaptPgPools
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're making great progress @Goddhi! 🚀
stats/lib/routes.js
Outdated
app.register(async app => { | ||
app.addHook('preHandler', filterPreHandlerHook) | ||
app.addHook('onSend', filterOnSendHook) | ||
|
||
app.get('/deals/daily', async (/** @type {RequestWithFilter} */ request, reply) => { | ||
const pgPools = adaptPgPools(request.server.pg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on changing the signature and dropping the adaptPgPools
function.
@juliangruber and @pyropy i have modified the fetcher functions signatures to accept Fastify's pg object directly and removed the adaptPgPools as suggested. |
"postgrator": "^8.0.0" | ||
}, | ||
"standard": { | ||
"env": [ | ||
"mocha" | ||
] | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
stats/bin/migrate.js
Outdated
@@ -6,4 +6,4 @@ import { | |||
|
|||
const pgPools = await getPgPools() | |||
await migrateStatsDB(pgPools.stats) | |||
await migrateEvaluateDB(pgPools.evaluate) | |||
await migrateEvaluateDB(pgPools.evaluate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await migrateEvaluateDB(pgPools.evaluate) | |
await migrateEvaluateDB(pgPools.evaluate) | |
stats/lib/app.js
Outdated
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stats/lib/platform-stats-fetchers.js
Outdated
|
||
const ONE_DAY = 24 * 60 * 60 * 1000 | ||
|
||
/** | ||
* @param {Queryable} pgPool | ||
* @param {Object} pg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param {Object} pg | |
* @param {Queryable} pg |
I believe this still follows the Queryable
interface. Object
is too unspecific.
stats/lib/platform-stats-fetchers.js
Outdated
|
||
/** | ||
* @param {FastifyPg} pg - Fastify pg object with database connections | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* @param {FastifyPg} pg - Fastify pg object with database connections | |
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
stats/lib/stats-fetchers.js
Outdated
@@ -35,7 +41,7 @@ export const fetchRetrievalSuccessRate = async (pg, filter) => { | |||
} | |||
|
|||
/** | |||
* @param {Object} pg | |||
* @param {any} pg - Fastify pg object with database connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param {any} pg - Fastify pg object with database connections | |
* @param {FastifyPg} pg - Fastify pg object with database connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
stats/lib/stats-fetchers.js
Outdated
@@ -399,7 +406,7 @@ export const fetchClientsRSRSummary = async (pg, filter) => { | |||
|
|||
/** | |||
* Fetches the retrieval stats summary for a single client for given date range. | |||
* @param {Object} pg | |||
* @param {{stats: Queryable, evaluate: Queryable}} pg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this not also be FastifyPg
?
@@ -1,10 +1,16 @@ | |||
import { FastifyRequest } from 'fastify' | |||
import { PostgresDb } from '@fastify/postgres' | |||
import { Queryable } from '@filecoin-station/spark-stats-db' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this type imported for?
@Goddhi CI is failing, please take a look |
FIXED |
@Goddhi CI is still failing. You can run |
i have fixed other issues i found after running test, but the only issue left is this |
The function is used here: spark-stats/stats/lib/platform-routes.js Line 23 in 43792c3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress 👍🏻
Let's clean the code up a bit; Let's try to pass database instance directly to the fetcher in which case we don't have to change fetcher signatures.
Also I'd advise you to format your code using standard
as that's what we use in majority of our repositories. You can do it simply by running npx standard --fix
.
stats/bin/spark-stats.js
Outdated
DATABASE_URL, | ||
EVALUATE_DB_URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add default values for both env
variables:
DATABASE_URL, | |
EVALUATE_DB_URL | |
DATABASE_URL = 'postgres://localhost:5432/spark_stats', | |
EVALUATE_DB_URL = 'postgres://localhost:5432/spark_evaluate' |
}) | ||
app.get('/stations/monthly', async (/** @type {RequestWithFilter} */ request, reply) => { | ||
reply.send(await fetchMonthlyStationCount(pgPools.evaluate, request.filter)) | ||
reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter)) | |
reply.send(await fetchMonthlyStationCount(request.server.pg.evaluate, request.filter)) |
We could also pass correct instance of the database instead of passing the pg
object. In that case we don't need to change fetchers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This throws an error reason why went with only the pg object, besides the fetcher functions expects full pg object and uses pg.stats or pg.evaluate interally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error does it throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
stats/lib/platform-stats-fetchers.js
Outdated
* @param {import('./typings.js').DateRangeFilter} filter | ||
*/ | ||
export const fetchDailyStationCount = async (pgPool, filter) => { | ||
const { rows } = await pgPool.query(` | ||
export const fetchDailyStationCount = async (pg, filter) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case we pass correct database instance we don't need to change this handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
}) | ||
app.get('/retrieval-result-codes/daily', async (/** @type {RequestWithFilter} */ request, reply) => { | ||
reply.send(await fetchDailyRetrievalResultCodes(pgPools, request.filter)) | ||
reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter)) | |
reply.send(await fetchDailyRetrievalResultCodes(request.server.pg.stats, request.filter)) |
We could also pass correct instance of the database instead of passing the pg object. In that case we don't need to change fetchers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
@pyropy and @juliangruber i have made a commits regarding your suggestions, kindly help review it thanks. |
}) | ||
app.get('/stations/monthly', async (/** @type {RequestWithFilter} */ request, reply) => { | ||
reply.send(await fetchMonthlyStationCount(pgPools.evaluate, request.filter)) | ||
reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error does it throw?
stats/lib/platform-stats-fetchers.js
Outdated
|
||
/** | ||
* @param {FastifyPg} pg - Fastify pg object with database connections | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
stats/lib/platform-stats-fetchers.js
Outdated
* @param {import('./typings.js').DateRangeFilter} filter | ||
*/ | ||
export const fetchDailyStationCount = async (pgPool, filter) => { | ||
const { rows } = await pgPool.query(` | ||
export const fetchDailyStationCount = async (pg, filter) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
}) | ||
app.get('/retrieval-result-codes/daily', async (/** @type {RequestWithFilter} */ request, reply) => { | ||
reply.send(await fetchDailyRetrievalResultCodes(pgPools, request.filter)) | ||
reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
@Goddhi tests are still failing, please run |
Yeah the reason the tests are failing is becuase i need to modify the test files to sync with the changes made in the fetcher and route files, this is what i am currently doing. |
#341 (comment) |
stats/test/app.test.js
Outdated
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') | ||
await app.pg.evaluate.query('DELETE FROM retrieval_stats') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's preserve pgPools
structure please so that we don't have to change every source code line in tests that is working with the DB.
For example:
before(async () => {
app = await createApp({
// ..
})
pgPools = app.pg
})
// no changes are needed in individual tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, noted
stats/lib/stats-fetchers.js
Outdated
@@ -472,12 +472,12 @@ export const fetchAllocatorsRSRSummary = async (pgPools, filter) => { | |||
|
|||
/** | |||
* Fetches the retrieval stats summary for a single allocator for given date range. | |||
* @param {PgPools} pgPools | |||
* @param {FastifyPg} pg - Fastify pg object with database connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the name pgPools
to keep the diff smaller?
* @param {FastifyPg} pg - Fastify pg object with database connections | |
* @param {FastifyPg} pgPools - Fastify pg object with database connections |
Same comment applies to all other similar places too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, noted
"postgrator": "^8.0.0" | ||
}, | ||
"standard": { | ||
"env": [ | ||
"mocha" | ||
] | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
stats/bin/migrate.js
Outdated
@@ -3,7 +3,6 @@ import { | |||
migrateEvaluateDB, | |||
migrateStatsDB | |||
} from '@filecoin-station/spark-stats-db' | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please undo this change to keep the diff clean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, noted
}) | ||
app.get('/stations/monthly', async (/** @type {RequestWithFilter} */ request, reply) => { | ||
reply.send(await fetchMonthlyStationCount(pgPools.evaluate, request.filter)) | ||
reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
}) | ||
app.get('/retrieval-result-codes/daily', async (/** @type {RequestWithFilter} */ request, reply) => { | ||
reply.send(await fetchDailyRetrievalResultCodes(pgPools, request.filter)) | ||
reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
stats/lib/stats-fetchers.js
Outdated
@@ -35,7 +41,7 @@ export const fetchRetrievalSuccessRate = async (pg, filter) => { | |||
} | |||
|
|||
/** | |||
* @param {Object} pg | |||
* @param {any} pg - Fastify pg object with database connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Goddhi
@pyropy Please can you help check what could be causing the CI to fail? |
Added fastify and @fastify/postgres as dependencies
Updated stats/bin/spark-stats.js to use Fastify's PostgreSQL plugin
Improved connection error handling and graceful shutdown
EDIT @juliangruber:
For CheckerNetwork/roadmap#220