Add healthcheck routes #688
Conversation
|
|
WalkthroughAdds two Next.js App Router GET endpoints: /ping returns plain text "OK"; /status reads package.json version at module load (fallback "unknown", logs on failure) and returns JSON { status: "OK", version }. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Ping as /ping route
participant Status as /status route
participant FS as package.json (fs)
rect rgba(224,240,255,0.4)
note over Status,FS: Module initialization (on module load)
Status->>FS: readFileSync(package.json)
FS-->>Status: version or error -> "unknown" (logged)
end
C->>Ping: GET /ping
Ping-->>C: 200 OK ("OK", text/plain, no-store)
C->>Status: GET /status
Status-->>C: 200 OK ({ status: "OK", version }, application/json, no-store)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
web-server/app/ping/route.ts (3)
3-7: Set explicit no-store caching for healthchecks.Prevents intermediary/CDN caching and ensures probes always hit the app.
Apply this diff:
export function GET() { - return new NextResponse('OK', { + return new NextResponse('OK', { status: 200, - headers: { 'Content-Type': 'text/plain' }, + headers: { + 'Content-Type': 'text/plain; charset=utf-8', + 'Cache-Control': 'no-store', + 'X-Robots-Tag': 'noindex', + }, }); }
3-8: Optional: add HEAD handler to reduce bandwidth for probes.Some platforms use HEAD; serving it avoids body bytes and log noise.
export function HEAD() { return new NextResponse(null, { status: 200, headers: { 'Cache-Control': 'no-store', 'X-Robots-Tag': 'noindex', }, }); }
3-7: Nit: use the standard Web Response to keep it minimal.Functionally identical here and avoids NextResponse-specific import.
-import { NextResponse } from "next/server"; +// No import needed if using global Response -export function GET() { - return new NextResponse('OK', { +export function GET() { + return new Response('OK', { status: 200, - headers: { 'Content-Type': 'text/plain' }, + headers: { + 'Content-Type': 'text/plain; charset=utf-8', + 'Cache-Control': 'no-store', + 'X-Robots-Tag': 'noindex', + }, }); }web-server/app/status/route.ts (5)
1-4: Force Node.js runtime; fs/path will fail on Edge.Prevents accidental deployment to Edge where
fsis unavailable.import { NextResponse } from 'next/server'; import { readFileSync } from 'fs'; import { join } from 'path'; +export const runtime = 'nodejs';
5-12: Tighten logging; avoid noisy raw error dumps.Prefer a concise warning with context; raw logs can leak paths.
} catch (err) { - console.log(err); + console.warn('status route: failed to read package.json for version', err instanceof Error ? err.message : err); }
14-18: Add no-store caching headers to status JSON.Health responses should not be cached.
-export async function GET() { - return NextResponse.json({ - status: 'OK', - version, - }); +export async function GET() { + return NextResponse.json( + { status: 'OK', version }, + { + headers: { + 'Cache-Control': 'no-store', + 'X-Robots-Tag': 'noindex', + }, + } + ); }
14-18: Optional: support HEAD for status.Useful for lightweight probes that only need headers.
export function HEAD() { return new Response(null, { status: 200, headers: { 'Cache-Control': 'no-store', 'X-Robots-Tag': 'noindex', }, }); }
7-9: Confirm package.json source — web-server/package.json present; no root package.jsonweb-server/package.json exists ([email protected]); no repository root package.json found — process.cwd() will resolve the intended file only if the app is started from the web-server directory. Resolve package.json relative to the route file or document the required working directory.
Location: web-server/app/status/route.ts (lines 7–9)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web-server/app/ping/route.ts(1 hunks)web-server/app/status/route.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
web-server/app/ping/route.ts (1)
web-server/app/status/route.ts (1)
GET(14-19)
web-server/app/status/route.ts (1)
web-server/app/ping/route.ts (1)
GET(3-8)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web-server/app/status/route.ts (2)
5-12: Make version sourcing env‑driven;process.cwd()path can be brittle in a monorepo.Relying on
process.cwd()/package.jsonmay read the workspace root instead of the app’s package, depending on how the server is started. Prefer an env var (e.g., APP_VERSION) with a safe fallback.-let version = 'unknown'; -try { - const pkgPath = join(process.cwd(), 'package.json'); - const pkgJson = JSON.parse(readFileSync(pkgPath, 'utf8')); - version = pkgJson.version || 'unknown'; -} catch (err) { - console.warn('status route: failed to read package.json for version', err instanceof Error ? err.message : err); -} +let version = process.env.APP_VERSION || process.env.npm_package_version || 'unknown'; +if (version === 'unknown') { + try { + const pkgPath = join(process.cwd(), 'package.json'); + const pkgJson = JSON.parse(readFileSync(pkgPath, 'utf8')); + version = typeof pkgJson.version === 'string' ? pkgJson.version : 'unknown'; + } catch (err) { + console.warn( + 'status route: failed to read package.json for version', + err instanceof Error ? err.message : err + ); + } +}Please confirm that
process.cwd()resolves to theweb-serverdirectory in all environments (dev/prod, local/CI).
14-14: Dropasync; no awaits inside.-export async function GET() { +export function GET() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web-server/app/ping/route.ts(1 hunks)web-server/app/status/route.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web-server/app/ping/route.ts
🧰 Additional context used
🧬 Code graph analysis (1)
web-server/app/status/route.ts (1)
web-server/app/ping/route.ts (1)
GET(3-12)
🔇 Additional comments (1)
web-server/app/status/route.ts (1)
16-18: Confirm policy on exposing version info.Returning the app version is useful for ops, but some orgs treat it as sensitive. Confirm it’s acceptable to expose publicly; otherwise gate it (e.g., behind auth or omit).
There was a problem hiding this comment.
While this is technically correct, this repo uses the pages router instead of the app router. (Unless I'm mistaken of course)
This change will be the only place that uses the app router style of declaring APIs.
Could you switch this to the pages router style of declaring APIs?
|
Also, we already have status based APIs. If you point your k8s livenessProbe path to one of these APIs, it should be just fine, right? |
|
Hey @jayantbh, thanks for the replies, I'll look into it. |
|
Your status route should be fine, it just need to respond a 200. I could create a page router route for async rewrites() {
return [
// Health check routes - map root paths to API routes
{
source: '/ping',
destination: '/api/ping',
},
{
source: '/status',
destination: '/api/status',
},
// ... any other existing rewrites ...
]
},However. I understand this is now very specific. While it doesn't change the existing, I understand you may not want to merge something like that. What do you think? My other options would be rebuilding from source injecting routes, injecting routes in Docker directly, fork the repo and make changes, or use a reverse proxy in the container. FYI, this is how our Dockerfile looks like: FROM middlewareeng/middleware:0.3.1
WORKDIR /app
COPY entrypoint.sh /app
ENTRYPOINT ["/app/entrypoint.sh"]
CMD ["/bin/bash", "-c", "/app/setup_utils/start.sh"]
|
|
Do you have the ability to specify query params to the endpoint in your framework? I might have an idea here then. Alternatively, we might be able to add a change that adds leverages some environment config to expose health endpoints on whatever path you specify. Or, to just keep things simple, pages router APIs for those cases are fine. I can't imagine we'll need to add another status API that has potential naming conflicts in a long time. |
Sadly no, but we're looking at a way to overcome this on our side as it is not really a "you" problem. Thanks for the efforts though, very much appreciated. I'll keep you posted. |
|
In case you're not able to get things to work on your end, we could try this:
|
|
@jayantbh I forgot to say, you might have a bunch of unwanted stuff in your image (0.3.1). Like a 233 MB zip file. /app# ls -lh
total 233M
-rw-r--r--. 1 root root 5.2K May 30 15:30 CODE_OF_CONDUCT.md
-rw-r--r--. 1 root root 3.7K May 30 15:30 SECURITY.md
drwxr-xr-x. 4 root root 120 May 30 15:30 backend
drwxr-xr-x. 3 root root 42 May 30 15:30 db
-rwxr-xr-x. 1 root root 295 Sep 12 14:52 entrypoint.sh
-rw-r--r--. 1 root root 566 May 30 15:30 env.example
-rwxr-xr-x. 1 root root 5.7K May 30 15:30 local-setup.sh
drwxr-xr-x. 2 root root 84 May 30 15:30 media_files
drwxr-xr-x. 1 root root 129 May 30 15:32 setup_utils
-rw-r--r--. 1 root root 124 May 30 15:30 version.txt
drwxr-xr-x. 1 root root 16K May 30 15:35 web-server
-rw-r--r--. 1 root root 233M May 30 15:35 web-server.tar.gz <-------
|
|
Sorry another side track comment, just an FYI cause the image is quite big, I'm seeing some things like this: 163M /app/web-server/node_modules/@next/swc-linux-x64-musl/next-swc.linux-x64-musl.node
134M /app/web-server/node_modules/@next/swc-linux-x64-gnu/next-swc.linux-x64-gnu.nodeYou may not need both, there is chance that this is only required for build a well (didn't check if your image is multi-stage). Just to let you know in case you want to optimize the size of the image, I'm getting that locally: dora latest c1aa3740c126 36 seconds ago 1.32GB |
|
I believe it is a multi-stage and multi-environment image. |
Yes sure, but that should still not be there I believe? These screenshots are taken from a deployed container. Discarded layers from different stages are not part of the image. |
|
Probably. I'll get it checked internally. Thanks for letting us know! :) |
Proposed changes (including videos or screenshots)
Add healthcheck routes that can be used with Kubernetes readiness processes.
Two routes have been added:
/pingthat return a plain text status 200/statusthat returns a json output status 200Further comments
This should be beneficial for anyone who wants to deployed the app in a Kubernetes setup. More info there:
https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/
Test with the following (change the URL to whatever it should be):
Summary by CodeRabbit