Conversation
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the Prometheus middleware by enabling it to accurately track and report metrics for HTTP HEAD requests. By aliasing HEAD requests to their GET counterparts, the middleware ensures that all relevant metrics, such as request counts, durations, and in-flight requests, are correctly captured. This provides a more robust and complete monitoring solution for applications using Fiber, ensuring no traffic goes unmeasured. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
gaby
changed the title
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Prometheus middleware for Fiber v3 and enhances it to correctly handle HEAD requests by aliasing them to GET routes for instrumentation. The implementation is robust and accompanied by a comprehensive test suite, which is excellent.
My review focuses on improving the usability and maintainability of the new middleware. I've provided two main points of feedback:
- A high-severity comment on the
README.mdto correct the usage example. The current example is misleading and would not instrument application routes as intended. I've suggested a corrected pattern that will help users adopt this middleware correctly. - A medium-severity comment in
config.gowith a suggestion to refactor the configuration initialization logic to reduce code duplication, improving maintainability.
Overall, this is a great contribution. Once the documentation is updated, this will be a very useful addition to the contrib repository.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Prometheus middleware for Fiber v3 that provides comprehensive HTTP metrics collection and exposure. The middleware instruments incoming HTTP traffic and exposes metrics via a configurable endpoint.
Key Changes:
- Implements a Prometheus middleware that records request/response metrics including counters, histograms, and gauges
- Provides configurable options for custom registries, histogram buckets, OpenMetrics support, and selective metric filtering
- Includes comprehensive test coverage with 22 test cases covering various configuration scenarios
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/prometheus/prometheus.go | Core middleware implementation with metrics collection, route tracking, and OpenTelemetry trace exemplar support |
| v3/prometheus/config.go | Configuration structure with default values and validation for middleware customization |
| v3/prometheus/prometheus_test.go | Comprehensive test suite covering all major features and edge cases |
| v3/prometheus/README.md | User documentation with usage examples and configuration options |
| v3/prometheus/go.mod | Module definition with dependency declarations |
| v3/prometheus/go.sum | Dependency checksums |
| .github/workflows/test-prometheus.yml | CI workflow for automated testing |
| v3/README.md | Updated to list the new prometheus middleware |
| README.md | Updated to list the new prometheus middleware |
| go.work | Added prometheus module to workspace |
💡 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 introduces a new Prometheus middleware for Fiber v3. The implementation is comprehensive, including a wide range of metrics, configuration options, and thorough tests. My review focuses on a critical issue in the documentation's usage example which would prevent the middleware from working as intended, and a suggestion to modernize the code in the configuration handling by using newer standard library packages for better readability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| app.Use("/metrics", fiberprometheus.New(fiberprometheus.Config{ | ||
| Service: "my-service-name", | ||
| SkipURIs: []string{"/ping"}, | ||
| IgnoreStatusCodes: []int{401, 403, 404}, | ||
| })) | ||
|
|
There was a problem hiding this comment.
The example mounts the middleware only on /metrics (app.Use("/metrics", ... )). With the current implementation, mounting only on the metrics path means non-metrics routes will never pass through the middleware, so requests to /, /ping, etc. won't be instrumented. Consider updating the example to mount the handler globally (for instrumentation) and mount it on the metrics path (for scraping), as done in the tests.
| app.Use("/metrics", fiberprometheus.New(fiberprometheus.Config{ | |
| Service: "my-service-name", | |
| SkipURIs: []string{"/ping"}, | |
| IgnoreStatusCodes: []int{401, 403, 404}, | |
| })) | |
| prometheus := fiberprometheus.New(fiberprometheus.Config{ | |
| Service: "my-service-name", | |
| SkipURIs: []string{"/ping"}, | |
| IgnoreStatusCodes: []int{401, 403, 404}, | |
| }) | |
| app.Use(prometheus.Middleware) | |
| app.Get("/metrics", prometheus.Handler()) |
| method := utils.CopyString(ctx.Method()) | ||
| routePath := m.resolveRoutePath(ctx) | ||
| routeKey := method + " " + routePath | ||
|
|
||
| registered := m.refreshRoutes(ctx, routeKey) | ||
| trackUnmatched := false | ||
| if !registered && m.cfg.TrackUnmatchedRequests { | ||
| routePath = normalizePath(m.cfg.UnmatchedRouteLabel) | ||
| trackUnmatched = true | ||
| } |
There was a problem hiding this comment.
routePath/routeKey are resolved before ctx.Next(). In Fiber v3, c.Route().Path is commonly only reliable after Next() (see v3/otel/fiber.go where the route is read after Next), and using ctx.Path() as the fallback here can lead to (1) high-cardinality metrics for parameterized routes (e.g. /users/123) and (2) failing to recognize registered routes when the router template is /users/:id. Consider resolving the normalized route path after ctx.Next() (using ctx.Route().Path when available) and using that for labels/registration checks to keep label cardinality bounded.
| gauge := promauto.With(registry).NewGaugeVec( | ||
| prometheus.GaugeOpts{ | ||
| Name: prometheus.BuildFQName(cfg.Namespace, cfg.Subsystem, "requests_in_progress_total"), | ||
| Help: "All the requests in progress", | ||
| ConstLabels: labels, | ||
| }, | ||
| []string{"method", "path"}, | ||
| ) |
There was a problem hiding this comment.
The in-flight metric is implemented as a GaugeVec but the name uses the _total suffix (requests_in_progress_total). In Prometheus naming conventions _total is reserved for monotonically increasing counters, while gauges typically omit _total (e.g. requests_in_progress). Since this is a new public metric, consider renaming now to avoid confusing dashboards/recording rules.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary