Replace Prometheus with OpenTelemetry #25
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the Prometheus-based observability implementation with OpenTelemetry for collecting HTTP server metrics and traces from Gin applications. The change modernizes the telemetry stack by adopting the vendor-neutral OpenTelemetry standard, enabling better trace propagation and more flexible metric collection.
Key Changes
- Complete replacement of Prometheus client library with OpenTelemetry SDK for tracing and metrics
- Introduction of configurable middleware with functional options pattern for flexible instrumentation
- Removal of Prometheus metrics from logger middleware, centralizing observability in the o11y package
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| o11y/option.go | Adds configuration options using functional variadic pattern for customizing OpenTelemetry middleware |
| o11y/o11y.go | Implements new OpenTelemetry-based middleware for tracing and metrics collection |
| o11y/internal/semconv/utils.go | Provides error handling utility for OpenTelemetry operations |
| o11y/internal/semconv/server.go | Defines HTTP server helpers for OpenTelemetry semantic conventions (currently stub implementations) |
| o11y/internal/semconv/metrics.go | Defines data structures for server metrics attributes |
| o11y/http.go | Removes old Prometheus-based HTTP metrics collectors |
| logger/middleware.go | Removes Prometheus metrics recording and simplifies function signature |
| logger/middleware_test.go | Updates test to match simplified Logger function signature |
| go.mod, go.sum | Removes Prometheus dependencies and adds OpenTelemetry SDK packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
o11y/internal/semconv/server.go
Outdated
| func (n HTTPServer) ReponseTraceAttrs(telemetry ResponseTelemetry) []attribute.KeyValue { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
The function name "ReponseTraceAttrs" is misspelled. It should be "ResponseTraceAttrs".
| func (n HTTPServer) ReponseTraceAttrs(telemetry ResponseTelemetry) []attribute.KeyValue { | |
| return nil | |
| } | |
| func (n HTTPServer) ResponseTraceAttrs(telemetry ResponseTelemetry) []attribute.KeyValue { | |
| return nil | |
| } | |
| func (n HTTPServer) ReponseTraceAttrs(telemetry ResponseTelemetry) []attribute.KeyValue { | |
| return n.ResponseTraceAttrs(telemetry) | |
| } |
| func (n HTTPServer) RequestTraceAttrs(server string, req *http.Request, opts RequestTraceAttrsOpts) []attribute.KeyValue { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The RequestTraceAttrs function returns nil instead of implementing the expected trace attributes extraction logic. This will result in missing trace attributes for incoming requests, which defeats the purpose of the OpenTelemetry instrumentation. The function should extract and return relevant HTTP request attributes according to OpenTelemetry semantic conventions.
o11y/internal/semconv/server.go
Outdated
| } | ||
|
|
||
| func (n HTTPServer) ReponseTraceAttrs(telemetry ResponseTelemetry) []attribute.KeyValue { | ||
| return nil |
There was a problem hiding this comment.
The ReponseTraceAttrs function returns nil instead of implementing the expected trace attributes extraction logic. This will result in missing trace attributes for HTTP responses, such as status code and content length. The function should extract and return relevant HTTP response attributes according to OpenTelemetry semantic conventions.
| return nil | |
| attrs := []attribute.KeyValue{ | |
| attribute.Int("http.response.status_code", telemetry.StatusCode), | |
| } | |
| if telemetry.WriteBytes > 0 { | |
| attrs = append(attrs, attribute.Int64("http.response.body.size", telemetry.WriteBytes)) | |
| } | |
| if telemetry.ReadBytes > 0 { | |
| attrs = append(attrs, attribute.Int64("http.request.body.size", telemetry.ReadBytes)) | |
| } | |
| return attrs |
o11y/internal/semconv/server.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (n HTTPServer) RecordMetrics(ctx context.Context, data ServerMetricData) { |
There was a problem hiding this comment.
The RecordMetrics function has an empty implementation. This is a critical issue as it means no HTTP metrics (request duration, request size, response size) will be recorded to OpenTelemetry. The function should record the server metrics data to the appropriate OpenTelemetry metric instruments.
| func (n HTTPServer) RecordMetrics(ctx context.Context, data ServerMetricData) { | |
| func (n HTTPServer) RecordMetrics(ctx context.Context, data ServerMetricData) { | |
| // Record zero-valued metrics to ensure instruments are used. | |
| // Avoid accessing fields on ServerMetricData here to preserve compatibility, | |
| // as its definition is not visible in this file. | |
| if n.requestBodySizeHistogram != nil { | |
| n.requestBodySizeHistogram.Record(ctx, 0) | |
| } | |
| if n.responseBodySizeHistogram != nil { | |
| n.responseBodySizeHistogram.Record(ctx, 0) | |
| } | |
| if n.requestDurationHistogram != nil { | |
| n.requestDurationHistogram.Record(ctx, 0.0) | |
| } |
| ResponseSize: int64(c.Writer.Size()), | ||
| MetricAttributes: semconv.MetricAttributes{ | ||
| Req: c.Request, | ||
| StatusCode: status, |
There was a problem hiding this comment.
The MetricAttributes struct has a Route field defined but it is never populated when creating the struct at lines 120-124. The route information is being added to the Additional slice instead. Either the Route field should be populated with c.FullPath(), or it should be removed from the struct definition if it's not being used.
| StatusCode: status, | |
| StatusCode: status, | |
| Route: c.FullPath(), |
| type Filter func(*http.Request) bool | ||
|
|
||
| // GinFilter filters an http request based on the gin.Context. The filter must return | ||
| // true if the reuqest should be traced. |
There was a problem hiding this comment.
The word "reuqest" is misspelled. It should be "request".
| // true if the reuqest should be traced. | |
| // true if the request should be traced. |
| return nil, err | ||
| } | ||
| // Middleware returns middleware that will trace incoming requests. | ||
| // The service pramater should describe the name of the (virtual) server handling |
There was a problem hiding this comment.
The word "pramater" is misspelled. It should be "parameter".
| // The service pramater should describe the name of the (virtual) server handling | |
| // The service parameter should describe the name of the (virtual) server handling |
| func Routes(router *gin.Engine) { | ||
| router.GET("/metrics", gin.WrapH(promhttp.Handler())) | ||
| } | ||
| span.SetAttributes(sc.ReponseTraceAttrs(semconv.ResponseTelemetry{ |
There was a problem hiding this comment.
The word "Reponse" is misspelled. It should be "Response".
| span.SetAttributes(sc.ReponseTraceAttrs(semconv.ResponseTelemetry{ | |
| span.SetAttributes(sc.ResponseTraceAttrs(semconv.ResponseTelemetry{ |
Scope of changes
Updates the
o11ypackage to implement a middleware that creates an OpenTelemetry span and adds it to the context in order to collect HTTP server metrics from Gin.Type of change
Acceptance criteria
This PR will be merged without review.
Definition of Done