Follow-up items from PR #165 (feat/metrics-stack)
These items were identified during the code review of PR #165 but are non-blocking and deferred to a follow-up.
Requested by @yash-pouranik.
1. admin.metrics.controller.js — Cohort accuracy & memory
- Duplicate signup events inflate cohortSize:
getCohorts counts all signup_completed events for a given month but a developer can emit that event more than once (e.g. multiple OAuth logins), causing the cohort denominator to be inflated. Deduplicate by developerId before counting.
- N+1 queries in cohort retention: The retention check loops per cohort member and issues individual
DeveloperActivity.findOne calls. Replace with a single $in query for all developer IDs and resolve retention in memory.
$addToSet memory risk on large datasets: The feature-usage aggregation uses $addToSet: '$developerId' in-memory in the aggregation pipeline, which can exceed the 100 MB aggregation memory limit for large collections. Consider using $count with a $group stage or a $facet approach instead.
2. apps/dashboard-api/src/routes/admin.metrics.js — Admin guard at router level
The admin authorization check (requireAdmin) is applied inside each controller handler rather than as route-level middleware. A future endpoint added to this router could silently bypass the guard. Move requireAdmin (or an equivalent middleware) to the router level so it applies automatically to all routes under /api/admin/metrics.
3. packages/common/src/queues/reliabilityAlertQueue.js — Doc comment mismatch
The JSDoc/inline comment describing the error-rate spike threshold states >50 requests but the actual code guard is >= 20 requests. Update the comment to match the real threshold to avoid confusion for future maintainers.
Backlinked from PR #165 comment: #165
Follow-up items from PR #165 (feat/metrics-stack)
These items were identified during the code review of PR #165 but are non-blocking and deferred to a follow-up.
Requested by @yash-pouranik.
1.
admin.metrics.controller.js— Cohort accuracy & memorygetCohortscounts allsignup_completedevents for a given month but a developer can emit that event more than once (e.g. multiple OAuth logins), causing the cohort denominator to be inflated. Deduplicate bydeveloperIdbefore counting.DeveloperActivity.findOnecalls. Replace with a single$inquery for all developer IDs and resolve retention in memory.$addToSetmemory risk on large datasets: The feature-usage aggregation uses$addToSet: '$developerId'in-memory in the aggregation pipeline, which can exceed the 100 MB aggregation memory limit for large collections. Consider using$countwith a$groupstage or a$facetapproach instead.2.
apps/dashboard-api/src/routes/admin.metrics.js— Admin guard at router levelThe admin authorization check (
requireAdmin) is applied inside each controller handler rather than as route-level middleware. A future endpoint added to this router could silently bypass the guard. MoverequireAdmin(or an equivalent middleware) to the router level so it applies automatically to all routes under/api/admin/metrics.3.
packages/common/src/queues/reliabilityAlertQueue.js— Doc comment mismatchThe JSDoc/inline comment describing the error-rate spike threshold states
>50 requestsbut the actual code guard is>= 20requests. Update the comment to match the real threshold to avoid confusion for future maintainers.Backlinked from PR #165 comment: #165