Gin Context implementation breaks context.Context contract

[dlinkov-roblox]

Description

As also described in this issue, open-telemetry/opentelemetry-go-contrib#5277, the existing OpenTelemetry packages do not interact very well with gin.Context. It was pointed out here that this is an example of gin.Context not fulfilling the following specification of context.Context.WithValue: The provided key must be comparable and should not be of type string or any other built-in type to avoid collisions between packages using context. Users of WithValue should define their own types for keys.

The two pieces of code that are having colliding values in the context are here for OpenTelemetry, and here for Gin.

How to reproduce

package main

import (
	"net/http"

	"github.com/gin-gonic/gin"
	"go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin"
	"go.opentelemetry.io/otel"
	"go.opentelemetry.io/otel/exporters/stdout/stdouttrace"
	oteltrace "go.opentelemetry.io/otel/sdk/trace"
	"go.opentelemetry.io/otel/trace"
)

func main() {
	sampler := oteltrace.AlwaysSample()
	exporter, _ := stdouttrace.New(stdouttrace.WithPrettyPrint())

	tp := oteltrace.NewTracerProvider(
		oteltrace.WithBatcher(exporter),
		oteltrace.WithSampler(sampler),
	)
	otel.SetTracerProvider(tp)
	r := gin.New()
	r.GET("/health_check", otelgin.Middleware("example-service"), HealthCheckHandler)
	r.Run(":9000")
}

func HealthCheckHandler(c *gin.Context) {
	span := trace.SpanFromContext(c)
	c.JSON(http.StatusOK, gin.H{"span_id": span.SpanContext().SpanID()})
}

Expectations

$ curl http://localhost:9000/health_check
<A span ID that is not all zeros>

Actual result

$ curl -i http://localhost:9000/hello/world
<A span ID that is all zeros>

Note that this is an issue as the span context will not be propagated properly unless a user passes c.Request.Context() explicitly. Since a gin.Context implements the context.Context interface, this is a subtle bug that would not be caught by the type system.

Environment

• go version: 1.19
• gin version (or commit ref): 1.9.1
• operating system: Mac OS Sonoma

[FarmerChillax]

I also encountered this confusion. I don’t know why gin does this. It seems to be a legacy issue. I can’t find a place to use it in the source code.

Therefore I made changes in this PR(#3897) to be as compatible as possible. respect the context.WithValue contract well should better.

[FarmerChillax]

A more compatible approach might be like this？

// Value returns the value associated with this context for key, or nil
// if no value is associated with key. Successive calls to Value with
// the same key returns the same result.
func (c *Context) Value(key any) any {
	if key == 0 {
		val := c.Request.Context().Value(key)
		if val != nil {
			return val
		}
		return c.Request
	}
	if key == ContextKey {
		return c
	}
	if keyAsString, ok := key.(string); ok {
		if val, exists := c.Get(keyAsString); exists {
			return val
		}
	}
	if !c.hasRequestContext() {
		return nil
	}
	return c.Request.Context().Value(key)
}

[appleboy]

Fixed in #3897 Thanks @FarmerChillax

[FarmerChillax]

Hi @appleboy , this feature is very important to me, I would like to know when it will be released