Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reopen "Gin Context implementation breaks context.Context contract #3888" #4074

Closed
huangjilaiqin opened this issue Oct 14, 2024 · 8 comments
Closed

Comments

@huangjilaiqin
Copy link

this commit did not fix problem,just replace 0 to ContextRequestKey
#3897
image

this code can solve this problem
image

@huangjilaiqin
Copy link
Author

@FarmerChillax @appleboy check this please

@FarmerChillax
Copy link
Contributor

see: #3993 (comment)

@FarmerChillax
Copy link
Contributor

FarmerChillax commented Oct 14, 2024

this code can solve this problem

this is a break change, I would not recommend making such changes in a widely used framework.

And this doesn't seem to be the root cause (with otel)

@huangjilaiqin
Copy link
Author

this is the root cause, ContextRequestKey==0 make otel's func SpanFromContext can not get the Span but get the c.Request
image
image

@FarmerChillax
Copy link
Contributor

FarmerChillax commented Oct 15, 2024

In otel the currentSpanKey is Type of traceConrextKeyType. In here, you can set ContextWithFallback = true to solue this problem

see: #3993 (comment)

with Go Context standards:

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. To avoid allocating when
assigning to an interface{}, context keys often have concrete type struct{}. Alternatively, exported context key variables'
static type should be a pointer or interface.

ref: https://pkg.go.dev/context#WithValue

@FarmerChillax
Copy link
Contributor

FarmerChillax commented Oct 15, 2024

Minimum case, Hope this helps. Go playground: https://go.dev/play/p/V_-_k4qhbMK

package main

import (
	"context"
	"fmt"
	"log"
	"net/http"

	"github.com/gin-gonic/gin"
	"go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin"
	"go.opentelemetry.io/otel"
	sdktrace "go.opentelemetry.io/otel/sdk/trace"
	"go.opentelemetry.io/otel/trace"
)

var applicationName string = "demo"

func main() {
	otel.SetTracerProvider(sdktrace.NewTracerProvider(sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.AlwaysSample()))))
	app := gin.Default()
	app.ContextWithFallback = true
	app.Use(otelgin.Middleware(applicationName))

	app.GET("/", func(ctx *gin.Context) {
		traceID, spanID, isSampled := GetTraceInfo(ctx)
		fmt.Printf("traceID: %v; spanID: %v; isSampled: %v\n", traceID, spanID, isSampled)
	})

	go func() {
		if err := app.Run(":8080"); err != nil {
			log.Fatalln(err)
		}
	}()

	http.Get("http://localhost:8080/")

}

func GetTraceInfo(ctx context.Context) (traceID string, spanID string, isSampled bool) {
	spanCtx := trace.SpanContextFromContext(ctx)

	if spanCtx.HasTraceID() {
		traceID = spanCtx.TraceID().String()
	}
	if spanCtx.HasSpanID() {
		spanID = spanCtx.SpanID().String()
	}

	isSampled = spanCtx.IsSampled()

	return traceID, spanID, isSampled
}

In shell:

$ go run main.go
[GIN-debug] [WARNING] Creating an Engine instance with the Logger and Recovery middleware already attached.

[GIN-debug] [WARNING] Running in "debug" mode. Switch to "release" mode in production.
 - using env:   export GIN_MODE=release
 - using code:  gin.SetMode(gin.ReleaseMode)

traceID: 68e41a688d27fa3c9f895417414b6dc5; spanID: de7b716f74c755e8; isSampled: true
[GIN] 2024/10/15 - 09:57:34 | 200 |      67.291µs |       127.0.0.1 | GET      "/"

go mod:

module tmp

go 1.22.0

toolchain go1.22.5

require (
	github.com/gin-gonic/gin v1.10.0
	go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin v0.56.0
	go.opentelemetry.io/contrib/propagators/b3 v1.31.0
	go.opentelemetry.io/otel v1.31.0
	go.opentelemetry.io/otel/sdk v1.31.0
	go.opentelemetry.io/otel/trace v1.31.0
)

require (
	github.com/bytedance/sonic v1.12.3 // indirect
	github.com/bytedance/sonic/loader v0.2.0 // indirect
	github.com/cloudwego/base64x v0.1.4 // indirect
	github.com/cloudwego/iasm v0.2.0 // indirect
	github.com/gabriel-vasile/mimetype v1.4.5 // indirect
	github.com/gin-contrib/sse v0.1.0 // indirect
	github.com/go-logr/logr v1.4.2 // indirect
	github.com/go-logr/stdr v1.2.2 // indirect
	github.com/go-playground/locales v0.14.1 // indirect
	github.com/go-playground/universal-translator v0.18.1 // indirect
	github.com/go-playground/validator/v10 v10.22.1 // indirect
	github.com/goccy/go-json v0.10.3 // indirect
	github.com/google/uuid v1.6.0 // indirect
	github.com/json-iterator/go v1.1.12 // indirect
	github.com/klauspost/cpuid/v2 v2.2.8 // indirect
	github.com/leodido/go-urn v1.4.0 // indirect
	github.com/mattn/go-isatty v0.0.20 // indirect
	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
	github.com/modern-go/reflect2 v1.0.2 // indirect
	github.com/pelletier/go-toml/v2 v2.2.3 // indirect
	github.com/twitchyliquid64/golang-asm v0.15.1 // indirect
	github.com/ugorji/go/codec v1.2.12 // indirect
	go.opentelemetry.io/otel/metric v1.31.0 // indirect
	golang.org/x/arch v0.11.0 // indirect
	golang.org/x/crypto v0.28.0 // indirect
	golang.org/x/net v0.30.0 // indirect
	golang.org/x/sys v0.26.0 // indirect
	golang.org/x/text v0.19.0 // indirect
	google.golang.org/protobuf v1.35.1 // indirect
	gopkg.in/yaml.v3 v3.0.1 // indirect
)

@huangjilaiqin
Copy link
Author

yes, this really work ,thk

In otel the currentSpanKey is Type of traceConrextKeyType. In here, you can set ContextWithFallback = true to solue this problem

see: #3993 (comment)

with Go Context standards:

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. To avoid allocating when
assigning to an interface{}, context keys often have concrete type struct{}. Alternatively, exported context key variables'
static type should be a pointer or interface.

ref: https://pkg.go.dev/context#WithValue

@FarmerChillax
Copy link
Contributor

Add example in gin-gonic/examples#171 @appleboy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants