refactor: extract metrics formatter#543
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughPrometheus text exposition logic previously inlined in ChangesPrometheus Metrics Formatter Extraction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: New metrics test omitted from CI
- Added test/server/metrics.test.ts to the test:mock script in package.json so it runs in CI with npm run test:all
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 6b43341. Configure here.
This ensures the new Prometheus formatter test runs in CI with npm run test:all
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/server/metrics.test.ts`:
- Around line 10-49: The test "formats current MCP metrics" currently has
limited assertions and doesn't validate all the metrics being generated. Add
additional assert.match calls to verify the Prometheus exposition format for
memory usage metrics (such as memory_usage_bytes with appropriate labels for
rss, heapTotal, heapUsed, external, and arrayBuffers), and stateless-related
counters (such as gitlab_mcp_stateless_requests_total,
gitlab_mcp_stateless_auth_from_header_total,
gitlab_mcp_stateless_auth_from_sealed_sid_total,
gitlab_mcp_stateless_auth_failures_total, and
gitlab_mcp_stateless_sid_rotated_total) to ensure comprehensive validation of
the output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16b81d6e-3925-4c08-92c8-12d3e2d7d974
📒 Files selected for processing (3)
index.tsserver/metrics.tstest/server/metrics.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Cursor Bugbot Autofix
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
🪛 ast-grep (0.43.0)
index.ts
[warning] 13092-13094: Avoid allowing access to unintended directories or files
Context: app.get("/metrics", (_req: Request, res: Response) => {
res.type("text/plain; version=0.0.4").send(formatPrometheusMetrics(getMetricsSnapshot()));
})
Note: Security best practice.
(path-traversal-typescript)
🔇 Additional comments (6)
server/metrics.ts (3)
1-27: LGTM!
29-31: LGTM!
33-116: LGTM!index.ts (2)
202-202: LGTM!
13092-13094: LGTM!Note: The static analysis warning about path-traversal is a false positive. This endpoint is a hardcoded route that returns formatted metrics without accepting any user input for file paths.
Source: Linters/SAST tools
test/server/metrics.test.ts (1)
6-8: LGTM!
| test("formats current MCP metrics", () => { | ||
| const body = formatPrometheusMetrics({ | ||
| requestsProcessed: 2, | ||
| rejectedByRateLimit: 1, | ||
| rejectedByCapacity: 0, | ||
| authFailures: 3, | ||
| totalSessions: 4, | ||
| expiredSessions: 5, | ||
| activeSessions: 6, | ||
| authenticatedSessions: 7, | ||
| gitlabClientPool: { size: 8, maxSize: 100 }, | ||
| uptime: 9, | ||
| memoryUsage: { | ||
| rss: 10, | ||
| heapTotal: 11, | ||
| heapUsed: 12, | ||
| external: 13, | ||
| arrayBuffers: 14, | ||
| }, | ||
| statelessRequests: 15, | ||
| statelessAuthFromHeader: 16, | ||
| statelessAuthFromSealedSid: 17, | ||
| statelessAuthFailures: 18, | ||
| statelessSidRotated: 19, | ||
| config: { | ||
| maxSessions: 1000, | ||
| maxRequestsPerMinute: 60, | ||
| sessionTimeoutSeconds: 3600, | ||
| remoteAuthEnabled: true, | ||
| mcpOAuthEnabled: false, | ||
| statelessModeEnabled: false, | ||
| statelessRotationKey: false, | ||
| }, | ||
| }); | ||
|
|
||
| assert.match(body, /# HELP gitlab_mcp_requests_processed_total/); | ||
| assert.match(body, /gitlab_mcp_requests_processed_total 2/); | ||
| assert.match(body, /gitlab_mcp_requests_rejected_total\{reason="rate_limit"\} 1/); | ||
| assert.match(body, /gitlab_mcp_config_info\{[^}]*remote_auth_enabled="true"/); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
The test provides solid coverage of the core formatting logic. If you want to strengthen the test suite, consider adding assertions for additional metric lines (e.g., memory_usage_bytes labels, stateless counters) to ensure comprehensive validation of the Prometheus exposition format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/server/metrics.test.ts` around lines 10 - 49, The test "formats current
MCP metrics" currently has limited assertions and doesn't validate all the
metrics being generated. Add additional assert.match calls to verify the
Prometheus exposition format for memory usage metrics (such as
memory_usage_bytes with appropriate labels for rss, heapTotal, heapUsed,
external, and arrayBuffers), and stateless-related counters (such as
gitlab_mcp_stateless_requests_total,
gitlab_mcp_stateless_auth_from_header_total,
gitlab_mcp_stateless_auth_from_sealed_sid_total,
gitlab_mcp_stateless_auth_failures_total, and
gitlab_mcp_stateless_sid_rotated_total) to ensure comprehensive validation of
the output.
2 participants
Summary
Part of #516.
This keeps the metrics refactor slice behavior-preserving:
server/metrics.tsindex.tsVerification
rtk npm run buildnode --import tsx/esm --test test/server/metrics.test.tsrtk npm run test:remote-authNote
Low Risk
Behavior-preserving refactor of observability output with no auth or request-path changes.
Overview
Moves Prometheus label escaping and
/metricstext formatting out ofindex.tsintoserver/metrics.ts, with aMetricsSnapshottype andformatPrometheusMetrics(snapshot)so formatting is pure and testable.The
/metricshandler now passesgetMetricsSnapshot()into that helper; counters, snapshot assembly, and/metrics.jsonstay inindex.ts.Adds
test/server/metrics.test.ts(label escaping and output shape) and includes it intest:mock.Reviewed by Cursor Bugbot for commit a13bbde. Bugbot is set up for automated code reviews on this repo. Configure here.