Add query and metrics endpoints for grafana - #528
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
CORS scope, query correctness, input handling, scalability, API contract, and lockfile issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds authenticated Grafana datasource endpoints for querying notification delivery data.
Changes:
- Adds
/metricsand/queryendpoints backed by notification dispatch data. - Adds credentialed Grafana CORS support.
- Adds OpenAPI documentation and endpoint tests.
File summaries
| File | Description |
|---|---|
src/packages/database/repositories/notifications-repository.ts |
Adds windowed notification queries with dispatches. |
src/packages/database/index.ts |
Exports the new query options type. |
src/apps/backend/utils/test-utils/database.ts |
Adds a repository mock. |
src/apps/backend/routers/grafana/index.ts |
Implements Grafana endpoints. |
src/apps/backend/routers/grafana/index.test.ts |
Tests authentication, CORS, and responses. |
src/apps/backend/routers/docs/openapi/paths/index.ts |
Registers Grafana paths. |
src/apps/backend/routers/docs/openapi/paths/grafana.ts |
Documents Grafana operations. |
src/apps/backend/routers/docs/openapi/components/schemas/index.ts |
Registers Grafana schemas. |
src/apps/backend/routers/docs/openapi/components/schemas/grafana-query.ts |
Defines query payload schemas. |
src/apps/backend/routers/docs/openapi/components/schemas/grafana-metrics.ts |
Defines metrics schema. |
src/apps/backend/package.json |
Adds CORS dependencies. |
src/apps/backend/middleware/grafana-cors.ts |
Configures allowed Grafana origins. |
src/apps/backend/app.ts |
Mounts CORS and Grafana routes. |
bun.lock |
Locks CORS dependencies. |
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| ).listSendsWithDispatchesInWindow({ | ||
| from: dateRange.from, | ||
| to: dateRange.to, | ||
| limit: maxNotifications, |
There was a problem hiding this comment.
Do we know if this limit is likely to be reached? If it is, should we signal that the results were truncated so Grafana doesn’t present incomplete data as complete?
There was a problem hiding this comment.
I think we might reach the hard limit if the user selects a wide timeframe. I am adding a meta object containing a message that Grafana should use it when results are truncated (haven't tested it though)
There was a problem hiding this comment.
If there are exactly 5,000 results, we show a truncation warning even though none were omitted. Could we fetch maxNotifications + 1, use the extra row to detect truncation, and return only the first 5,000?
There was a problem hiding this comment.
Great catch!
Co-authored-by: mgosz-guardian <marcin.gosz@theguardian.com>
What does this change?
Add 2 new endpoints to be consumed by grafana: /metrics and /query
These will pull data from our database to grafana to show stats about notifications that have been sent.
This PR also adds CORS and adds Grafana to the allowed origins.
These are the columns we will display in the grafana visualistion:
How has this change been tested?
I can't seem to be able to test on CODE, there seems to be a problem with the cookie which does not get sent when Grafana makes the request to CODE.