-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: daily & monthly participants #10
Changes from all commits
26da540
3448a1d
ee1f1a0
da12e08
0533180
c334b2e
fd648d0
92ecc88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,7 +3,11 @@ import { json } from 'http-responders' | |||||
import Sentry from '@sentry/node' | ||||||
import { URLSearchParams } from 'node:url' | ||||||
|
||||||
import { fetchRetrievalSuccessRate } from './stats-fetchers.js' | ||||||
import { | ||||||
fetchDailyParticipants, | ||||||
fetchMonthlyParticipants, | ||||||
fetchRetrievalSuccessRate | ||||||
} from './stats-fetchers.js' | ||||||
|
||||||
/** | ||||||
* @param {object} args | ||||||
|
@@ -42,6 +46,20 @@ const handler = async (req, res, pgPool) => { | |||||
res, | ||||||
pgPool, | ||||||
fetchRetrievalSuccessRate) | ||||||
} else if (req.method === 'GET' && segs[0] === 'participants' && segs[1] === 'daily' && segs.length === 2) { | ||||||
await getStatsWithFilterAndCaching( | ||||||
pathname, | ||||||
searchParams, | ||||||
res, | ||||||
pgPool, | ||||||
fetchDailyParticipants) | ||||||
} else if (req.method === 'GET' && segs[0] === 'participants' && segs[1] === 'monthly' && segs.length === 2) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
no need to use segments if we have static urls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (also applies above and below) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I was wondering about this too. But then your suggestion changes how we handle paths like Maybe I can have a "normalised path string" created via WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it gets complicated like this, let's just switch to fastify 🤷♂️ I don't mind all these edge cases, since I don't yet see any problems arise from them. If you want this to be tighter, I propose let's just make the switch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TBH, I don't have an appetite to spend more time on this. I know the current code could be improved; maybe we can also simplify it by not worrying about inconsistencies in edge case handling. Either way, I feel it's a minor detail, and we have bigger fish to fry. I am going to land what we have now; I added the "Migrate our REST APIs to Fastify" task to our M4.2 milestone. |
||||||
await getStatsWithFilterAndCaching( | ||||||
pathname, | ||||||
searchParams, | ||||||
res, | ||||||
pgPool, | ||||||
fetchMonthlyParticipants) | ||||||
} else if (req.method === 'GET' && segs.length === 0) { | ||||||
// health check - required by Grafana datasources | ||||||
res.end('OK') | ||||||
|
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.