feat(monitor): add metrics, new features, and fix middleware bugs#1833
Conversation
Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/8ce24309-9791-42c3-8cfe-09157fce8acd Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/8ce24309-9791-42c3-8cfe-09157fce8acd Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
…nic on process error, APIOnly override logic Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/6da9d492-f179-4c24-9e49-d2afbdb03db7 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/487134b0-4d76-49f4-a3bd-06a4d75c4f12 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/6b4667ef-7e6d-4c21-a14b-a672ea5edde8 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
…nil-safe process, APIOnly default, improved test Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/5107777e-20e7-4972-a097-544f30f8fda9 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
…bal tests, typed JSON assertions, close resp.Body Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/50b76845-c4b0-4416-8204-1a577a58c8dc Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
…ts chart first-point Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/12a845e9-53a4-436f-8abd-ecff91228479 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
…data+mutex Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/77ad6db7-2000-4c9a-b60a-d2cb6a63abce Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
… assertions Agent-Logs-Url: https://github.com/gofiber/contrib/sessions/32b1ff60-29cc-45fc-979f-523680edd736 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Removed comments regarding APIOnly inheritance behavior.
📝 WalkthroughWalkthroughThe PR refactors the v3 monitor middleware to replace mutex-protected shared data with atomic.Value storage, adds three new metrics (Requests, Goroutines, Uptime) with frontend visualization, improves test coverage for request counting, and documents the application-wide counting pattern via the ChangesMonitor Middleware: Atomic Metrics & Dashboard Expansion
Sequence DiagramsequenceDiagram
participant Client
participant FiberApp
participant MonitorHandler as Monitor Handler
participant AtomicMetrics as Atomic Metrics
participant Dashboard as Dashboard/API
participant Frontend as Frontend JavaScript
Client->>FiberApp: Request to any path
FiberApp->>MonitorHandler: Invoke monitor middleware
MonitorHandler->>AtomicMetrics: Increment request counter
MonitorHandler->>MonitorHandler: Check Next filter
alt Next returns true (skip)
MonitorHandler->>FiberApp: Continue to next handler
else Next returns false (show dashboard)
MonitorHandler->>AtomicMetrics: Load PID metrics (requests, goroutines, uptime)
MonitorHandler->>AtomicMetrics: Load OS metrics
MonitorHandler->>Dashboard: Return JSON snapshot
Dashboard->>Client: JSON response
Client->>Frontend: Parse JSON data
Frontend->>Frontend: Calculate request delta (BigInt)
Frontend->>Frontend: Update metric displays & charts
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
There was a problem hiding this comment.
Pull request overview
Adds new monitor metrics (requests, goroutines, uptime), improves JSON safety/precision, and extends UI/tests/workflow coverage for the monitor middleware.
Changes:
- Add
pid.requests(uint64 encoded as string),pid.goroutines, andpid.uptimeto the monitor JSON payload. - Update the dashboard UI to display/chart requests and goroutines, and show uptime.
- Extend monitor tests and CI matrix (adds Go
1.26.x) to cover the new fields and behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
v3/monitor/monitor.go |
Adds atomic-backed request counter, uptime/goroutine metrics, and snapshot-based JSON response generation. |
v3/monitor/index.go |
Adds new UI elements and charts for requests/goroutines and an uptime display; updates JS sampling logic. |
v3/monitor/monitor_test.go |
Adds typed JSON assertions, request counter test, and improves response body handling. |
v3/monitor/config_test.go |
Adds coverage for ConfigDefault.APIOnly inheritance behavior. |
.github/workflows/test-monitor.yml |
Expands Go test matrix to include 1.26.x. |
v3/swaggerui/go.mod |
Removes trailing blank line at EOF. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request enhances the monitor middleware by adding Uptime, Total Requests, and Goroutine metrics, along with corresponding dashboard UI updates. It improves robustness by initializing atomic values to prevent panics and handling cases where process information is unavailable. Feedback indicates that the "Total Requests" counter is misleading as it only tracks requests to the monitor endpoint rather than the entire application, and notes potential performance concerns regarding frequent connection scanning.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…iddleware-absolute-counter
- Count requests before the Next check so the middleware can be mounted app-wide as a traffic collector; document the pattern in the README - Use textContent instead of innerHTML for plain-text dashboard metrics - Clamp the requests chart delta to [0, MAX_SAFE_INTEGER] to handle server restarts and uint64 precision loss in JavaScript - Replace defer Body.Close() with t.Cleanup assertions, dropping the nolint suppression - Add a test covering counting of Next-filtered requests - Fix the Next description in the README config table Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/monitor/monitor.go (1)
108-115:Nextskips monitor rendering but still increments “Total Requests” (intentional, documented + tested)
monitTotalRequests.Add(1)runs before thecfg.Nextshort-circuit, so requests returningtrueare still counted;v3/monitor/README.mdandTest_Monitor_Requests_NextFiltereddescribe this behavior.- The inline comment in
v3/monitor/monitor.go(“Don’t execute middleware if Next returns true”) is slightly misleading because only the monitor response logic is skipped, not the counter increment.🤖 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 `@v3/monitor/monitor.go` around lines 108 - 115, The inline comment in monitor.go is misleading: monitTotalRequests.Add(1) intentionally runs before the cfg.Next short-circuit so filtered requests are still counted; update the comment near monitTotalRequests.Add(1) and the subsequent if cfg.Next != nil && cfg.Next(c) block to explicitly state that cfg.Next only prevents the middleware response/rendering but does NOT prevent the total-request counter from incrementing (reference symbols: monitTotalRequests.Add and cfg.Next).
🤖 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.
Nitpick comments:
In `@v3/monitor/monitor.go`:
- Around line 108-115: The inline comment in monitor.go is misleading:
monitTotalRequests.Add(1) intentionally runs before the cfg.Next short-circuit
so filtered requests are still counted; update the comment near
monitTotalRequests.Add(1) and the subsequent if cfg.Next != nil && cfg.Next(c)
block to explicitly state that cfg.Next only prevents the middleware
response/rendering but does NOT prevent the total-request counter from
incrementing (reference symbols: monitTotalRequests.Add and cfg.Next).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b66004b-4e09-4e71-89fd-367de4bba70b
⛔ Files ignored due to path filters (1)
.github/workflows/test-monitor.ymlis excluded by!**/*.yml
📒 Files selected for processing (5)
v3/monitor/README.mdv3/monitor/config_test.gov3/monitor/index.gov3/monitor/monitor.gov3/monitor/monitor_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
v3/monitor/README.md (2)
41-41: LGTM!
73-84: LGTM!v3/monitor/monitor_test.go (4)
5-5: LGTM!Also applies to: 9-9
127-157: LGTM!
226-271: LGTM!
273-304: LGTM!v3/monitor/config_test.go (1)
165-184: LGTM!v3/monitor/index.go (5)
96-102: LGTM!Also applies to: 108-108, 146-163
178-186: LGTM!
216-218: LGTM!Also applies to: 224-225, 231-232, 234-234
265-265: LGTM!Also applies to: 285-295
267-268: LGTM!Also applies to: 275-277, 296-296
v3/monitor/monitor.go (1)
69-103: LGTM!Also applies to: 122-153, 160-195
All original changes preserved from the same branch.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests