Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
6755b18
fix: use HTTP method and path as span name instead of generic ServeHTTP
ankurs Mar 28, 2026
0b64e49
feat: record HTTP status code and error status on OTEL spans
ankurs Mar 28, 2026
91ad35b
refactor: upgrade semconv from v1.12.0 to v1.40.0
ankurs Mar 28, 2026
f705ea1
feat: use grpc-gateway route patterns for low-cardinality span names
ankurs Mar 28, 2026
03b739d
fix: capture first WriteHeader only, split server.address from port
ankurs Mar 29, 2026
96f996b
feat: add git commit, go version, and VCS info to OTEL resource
ankurs Mar 29, 2026
567810b
feat: add git commit, go version, and binary name to OTEL resource
ankurs Mar 29, 2026
ab78d2d
fix: handle 1xx responses and implicit Write in statusRecorder
ankurs Mar 29, 2026
c810545
fix: treat 101 Switching Protocols as final status in statusRecorder
ankurs Mar 29, 2026
d0cc95c
fix: update statusRecorder comment for 101, strip IPv6 brackets from …
ankurs Mar 29, 2026
e4c4757
fix: use runtime.HTTPPattern for route extraction, add span attribute…
ankurs Mar 29, 2026
be90f26
fix: guard empty server.address, find spans by name, add 5xx error test
ankurs Mar 29, 2026
3e48e72
fix: record 500 status on span when handler panics before writing hea…
ankurs Mar 29, 2026
9e698f8
fix: derive url.scheme from r.TLS instead of empty r.URL.Scheme
ankurs Mar 29, 2026
2edd9c7
merge: resolve conflicts with main, keep PR fixes
ankurs Mar 29, 2026
13df802
fix: check X-Forwarded-Proto for url.scheme behind TLS-terminating pr…
ankurs Mar 29, 2026
f5b473c
fix: always set span status Error on panic, assert url.scheme in test
ankurs Mar 29, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions config/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ import "github.com/go-coldbrew/core/config"


<a name="Config"></a>
## type [Config](<https://github.com/go-coldbrew/core/blob/main/config/config.go#L6-L137>)
## type [Config](<https://github.com/go-coldbrew/core/blob/main/config/config.go#L6-L138>)

Config is the configuration for the Coldbrew server It is populated from environment variables and has sensible defaults for all fields so that you can just use it as is without any configuration The following environment variables are supported and can be used to override the defaults for the fields

Expand Down Expand Up @@ -195,14 +195,15 @@ type Config struct {
// OTLPSamplingRatio is the ratio of traces to sample (0.0 to 1.0)
// 1.0 means sample all traces, 0.1 means sample 10% of traces
OTLPSamplingRatio float64 `envconfig:"OTLP_SAMPLING_RATIO" default:"0.2"`
// OTLPUseOpenTracingBridge determines whether to set up OpenTracing compatibility bridge
// This allows using existing OpenTracing instrumentation with OpenTelemetry
OTLPUseOpenTracingBridge bool `envconfig:"OTLP_USE_OPENTRACING_BRIDGE" default:"true"`
// Deprecated: OpenTracing bridge is provided for backwards compatibility only.
// New services should leave this false (the default). Set to true only if you
// have existing OpenTracing instrumentation that hasn't been migrated to OTEL.
OTLPUseOpenTracingBridge bool `envconfig:"OTLP_USE_OPENTRACING_BRIDGE" default:"false"`
}
```

<a name="Config.Validate"></a>
### func \(Config\) [Validate](<https://github.com/go-coldbrew/core/blob/main/config/config.go#L142>)
### func \(Config\) [Validate](<https://github.com/go-coldbrew/core/blob/main/config/config.go#L143>)

```go
func (c Config) Validate() []string
Expand Down
133 changes: 117 additions & 16 deletions core.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"net"
"net/http"
"net/http/pprof"
"strconv"
"strings"
"sync"
"time"
Expand All @@ -24,8 +25,10 @@ import (
"github.com/prometheus/client_golang/prometheus/promhttp"
"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/codes"
"go.opentelemetry.io/otel/propagation"
semconv "go.opentelemetry.io/otel/semconv/v1.12.0"
semconv "go.opentelemetry.io/otel/semconv/v1.40.0"
oteltrace "go.opentelemetry.io/otel/trace"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
Expand Down Expand Up @@ -169,37 +172,134 @@ func (c *cb) processConfig() {
}
}

// statusRecorder wraps http.ResponseWriter to capture the final HTTP status code.
// It records the first status >= 200, plus 101 Switching Protocols (which is
// terminal). Other 1xx statuses are informational and skipped.
// Unwrap() is provided for http.ResponseController (Go 1.20+) to access optional
// interfaces (http.Flusher, http.Hijacker, etc.) from the underlying writer.
type statusRecorder struct {
http.ResponseWriter
status int
wroteHeader bool
}

func (sr *statusRecorder) WriteHeader(code int) {
if !sr.wroteHeader && (code >= 200 || code == http.StatusSwitchingProtocols) {
sr.status = code
sr.wroteHeader = true
}
sr.ResponseWriter.WriteHeader(code)
}

func (sr *statusRecorder) Write(b []byte) (int, error) {
if !sr.wroteHeader {
sr.status = http.StatusOK
sr.wroteHeader = true
}
return sr.ResponseWriter.Write(b)
}

// Unwrap returns the underlying ResponseWriter so that http.ResponseController
// and middleware can access optional interfaces (http.Flusher, http.Hijacker, etc.).
func (sr *statusRecorder) Unwrap() http.ResponseWriter {
return sr.ResponseWriter
}

// endSpan records the HTTP status code on the span, marks it as error for 5xx, and ends it.
func endSpan(span oteltrace.Span, rec *statusRecorder) {
span.SetAttributes(semconv.HTTPResponseStatusCode(rec.status))
if rec.status >= 500 {
span.SetStatus(codes.Error, http.StatusText(rec.status))
}
span.End()
Comment thread
ankurs marked this conversation as resolved.
}

// httpSpanAttributes returns the OTEL attributes for an incoming HTTP request,
// omitting empty-valued attributes (e.g. scheme behind a reverse proxy).
func httpSpanAttributes(r *http.Request) []attribute.KeyValue {
host, port, err := net.SplitHostPort(r.Host)
if err != nil {
host = r.Host
}
host = strings.TrimPrefix(strings.TrimSuffix(host, "]"), "[")
attrs := []attribute.KeyValue{
semconv.HTTPRequestMethodKey.String(r.Method),
semconv.URLPath(r.URL.Path),
}
if host != "" {
attrs = append(attrs, semconv.ServerAddress(host))
}
if port != "" {
if p, err := strconv.Atoi(port); err == nil {
attrs = append(attrs, semconv.ServerPort(p))
}
}
if r.URL.RawQuery != "" {
attrs = append(attrs, semconv.URLQuery(r.URL.RawQuery))
}
scheme := r.URL.Scheme
if scheme == "" {
if r.TLS != nil {
scheme = "https"
} else {
scheme = "http"
}
}
attrs = append(attrs, semconv.URLScheme(scheme))
Comment on lines +246 to +250

Copilot AI Mar 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

httpSpanAttributes is documented as "omitting empty-valued attributes", but the new logic always appends url.scheme (defaulting to "http"). Either update the function comment to match the new behavior, or only set url.scheme when it’s explicitly known (URL scheme provided, TLS present, or a trusted forwarded-proto header).

Suggested change
} else {
scheme = "http"
}
}
attrs = append(attrs, semconv.URLScheme(scheme))
}
}
if scheme != "" {
attrs = append(attrs, semconv.URLScheme(scheme))
}

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the comment — url.scheme is now always set (derived from X-Forwarded-Proto / r.TLS / default http). The function comment should be updated. Will fix if we do another pass on this PR.

return attrs
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// tracingWrapper is a middleware that creates a new OTEL span for each incoming HTTP request.
// It extracts any propagated trace context from the request headers and, for non-filtered
// methods, starts a server span that is attached to the request context.
func tracingWrapper(h http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
prop := otel.GetTextMapPropagator()
ctx := prop.Extract(r.Context(), propagation.HeaderCarrier(r.Header))
ctx := otel.GetTextMapPropagator().Extract(r.Context(), propagation.HeaderCarrier(r.Header))

if interceptors.FilterMethodsFunc(ctx, r.URL.Path) {
var serverSpan oteltrace.Span
ctx, serverSpan = otel.Tracer("coldbrew-http").Start(ctx, "ServeHTTP",
ctx, serverSpan = otel.Tracer("coldbrew-http").Start(ctx, r.Method,
oteltrace.WithSpanKind(oteltrace.SpanKindServer),
oteltrace.WithAttributes(
semconv.HTTPMethodKey.String(r.Method),
semconv.HTTPTargetKey.String(r.URL.RequestURI()),
),
oteltrace.WithAttributes(httpSpanAttributes(r)...),
)
r = r.WithContext(ctx)
defer serverSpan.End()
} else {
r = r.WithContext(ctx)
rec := &statusRecorder{ResponseWriter: w, status: http.StatusOK}
w = rec
defer func() {
if recovered := recover(); recovered != nil {
if !rec.wroteHeader {
rec.status = http.StatusInternalServerError
}
serverSpan.RecordError(fmt.Errorf("panic: %v", recovered))
Comment thread
ankurs marked this conversation as resolved.
endSpan(serverSpan, rec)
panic(recovered)
}
endSpan(serverSpan, rec)
Comment on lines +269 to +279

Copilot AI Mar 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new panic recovery path updates the span (RecordError/SetStatus) and may force a 500 status, but there’s no test covering this behavior. Add a test that triggers a panic in the wrapped handler, asserts the request panics (recovered in the test), and verifies the exported span has http.response.status_code=500 and Status.Code==Error (and ideally an error event/recorded error).

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — a panic recovery test would strengthen coverage. Adding OTel SDK as a test dep already enabled span assertions; a panic test is a good follow-up alongside the existing TestTracingWrapperSpanErrorStatus.

}()
}

_, han := interceptors.NRHttpTracer("", h.ServeHTTP)
// add this info to log
ctx = r.Context()
ctx = options.AddToOptions(ctx, "", "")
ctx = loggers.AddToLogContext(ctx, "httpPath", r.URL.Path)
r = r.WithContext(ctx)
han(w, r)
han(w, r.WithContext(ctx))
})
}

// spanRouteMiddleware is a grpc-gateway middleware that updates the OTEL span
// name and http.route attribute with the matched route pattern after routing.
// It uses runtime.HTTPPattern (the Pattern struct set by handleHandler) rather
// than runtime.HTTPPathPattern (the string set later inside AnnotateContext).
func spanRouteMiddleware(next runtime.HandlerFunc) runtime.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request, pathParams map[string]string) {
if pattern, ok := runtime.HTTPPattern(r.Context()); ok {
route := pattern.String()
span := oteltrace.SpanFromContext(r.Context())
span.SetName(r.Method + " " + route)
span.SetAttributes(semconv.HTTPRoute(route))
}
next(w, r, pathParams)
}
}

// getCustomHeaderMatcher returns a matcher that matches the given header and prefix
func getCustomHeaderMatcher(prefixes []string, header string) func(string) (string, bool) {
header = strings.ToLower(header)
Expand Down Expand Up @@ -239,6 +339,7 @@ func (c *cb) initHTTP(ctx context.Context) (*http.Server, error) {
),
runtime.WithMarshalerOption("application/proto", pMar),
runtime.WithMarshalerOption("application/protobuf", pMar),
runtime.WithMiddlewares(spanRouteMiddleware),
}

if c.config.UseJSONBuiltinMarshaller {
Expand Down
Loading
Loading