From 71c2fd0110d0df07e3af7acdbb439b0fa3c88f35 Mon Sep 17 00:00:00 2001 From: Gerry Agbobada Date: Mon, 6 Nov 2023 12:03:23 +0100 Subject: [PATCH] Fix context replacement for http.Request The context of a HTTP request is not assignable, so we need to read from the existing context and create a new one --- CHANGELOG.md | 9 +++++---- examples/otel/cmd/main.go | 8 ++++---- examples/web/cmd/main.go | 4 ++-- internal/autometrics/ctx.go | 12 ++++++++---- internal/generate/context.go | 10 ++++++++-- internal/generate/defer_test.go | 12 ++++++------ 6 files changed, 33 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2692501..d5fe324 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,10 +27,11 @@ specification. tracking. If `autometrics` detects a `context.Context` it can use, it will shadow the context with the autometrics augmented one to reduce the changes to make in the code. Currently, `autometrics` can detect arguments of specific types in the function signatures - to replace contexts: - + `context.Context` - + `http.Request` - + `buffalo.Request` + to read from and replace contexts: + + `context.Context` (read and replace) + + `http.Request` (read only) + + `buffalo.Request` (read and replace) + + `gin.Context` (read only; _very_ experimental) - [Generator] The generator now tries to keep going with instrumentation even if some instrumentation fails. On error, still _No file will be modified_, and the generator will exit with an error code and output all the collected errors instead of only the first one. diff --git a/examples/otel/cmd/main.go b/examples/otel/cmd/main.go index d15c939..60a63ed 100644 --- a/examples/otel/cmd/main.go +++ b/examples/otel/cmd/main.go @@ -98,14 +98,14 @@ func main() { // //autometrics:doc --slo "API" --latency-target 99 --latency-ms 5 func indexHandler(w http.ResponseWriter, r *http.Request) error { - r.Context() = autometrics.PreInstrument(autometrics.NewContext( + amCtx := autometrics.PreInstrument(autometrics.NewContext( r.Context(), autometrics.WithConcurrentCalls(true), autometrics.WithCallerName(true), autometrics.WithSloName("API"), autometrics.WithAlertLatency(5000000*time.Nanosecond, 99), )) //autometrics:shadow-ctx - defer autometrics.Instrument(r.Context(), nil) //autometrics:defer + defer autometrics.Instrument(amCtx, nil) //autometrics:defer msSleep := rand.Intn(200) time.Sleep(time.Duration(msSleep) * time.Millisecond) @@ -149,14 +149,14 @@ var handlerError = errors.New("failed to handle request") // //autometrics:doc --slo "API" --success-target 90 func randomErrorHandler(w http.ResponseWriter, r *http.Request) (err error) { - r.Context() = autometrics.PreInstrument(autometrics.NewContext( + amCtx := autometrics.PreInstrument(autometrics.NewContext( r.Context(), autometrics.WithConcurrentCalls(true), autometrics.WithCallerName(true), autometrics.WithSloName("API"), autometrics.WithAlertSuccess(90), )) //autometrics:shadow-ctx - defer autometrics.Instrument(r.Context(), &err) //autometrics:defer + defer autometrics.Instrument(amCtx, &err) //autometrics:defer isOk := rand.Intn(10) == 0 diff --git a/examples/web/cmd/main.go b/examples/web/cmd/main.go index 8a3868b..99e0485 100644 --- a/examples/web/cmd/main.go +++ b/examples/web/cmd/main.go @@ -107,14 +107,14 @@ func main() { // //autometrics:inst --slo "API" --latency-target 99 --latency-ms 5 func indexHandler(w http.ResponseWriter, r *http.Request) error { - r.Context() = autometrics.PreInstrument(autometrics.NewContext( + amCtx := autometrics.PreInstrument(autometrics.NewContext( r.Context(), autometrics.WithConcurrentCalls(true), autometrics.WithCallerName(true), autometrics.WithSloName("API"), autometrics.WithAlertLatency(5000000*time.Nanosecond, 99), )) //autometrics:shadow-ctx - defer autometrics.Instrument(r.Context(), nil) //autometrics:defer + defer autometrics.Instrument(amCtx, nil) //autometrics:defer msSleep := rand.Intn(200) time.Sleep(time.Duration(msSleep) * time.Millisecond) diff --git a/internal/autometrics/ctx.go b/internal/autometrics/ctx.go index 77db792..1b50b88 100644 --- a/internal/autometrics/ctx.go +++ b/internal/autometrics/ctx.go @@ -47,8 +47,11 @@ type GeneratorContext struct { // only statically-known information is useful at this point. type RuntimeCtxInfo struct { // Name of the variable to use as context.Context when building the autometrics.Context. - // The string will be empty if and only if 'nil' must be used as autometrics.NewContext() argument. + // The string will be empty if 'nil' must be used as autometrics.NewContext() argument. ContextVariableName string + // Name of the variable to assign to the context returned by autometrics.PreInstrument + // The string will be empty if a new variable must be used as autometrics.PreInstrument() return value assignment. + NewContextVariableName string // Verbatim code to use to fetch the TraceID. // For example, if the instrumented function is detected to use Gin, like // `func ginHandler(ginVarName *gin.Context)` @@ -65,9 +68,10 @@ type RuntimeCtxInfo struct { func DefaultRuntimeCtxInfo() RuntimeCtxInfo { return RuntimeCtxInfo{ - TrackConcurrentCalls: true, - TrackCallerName: true, - ContextVariableName: "nil", + TrackConcurrentCalls: true, + TrackCallerName: true, + ContextVariableName: "nil", + NewContextVariableName: "", } } diff --git a/internal/generate/context.go b/internal/generate/context.go index e21ede4..1d220be 100644 --- a/internal/generate/context.go +++ b/internal/generate/context.go @@ -148,8 +148,8 @@ func buildAutometricsContextNode(agc *internal.GeneratorContext) (newContextCall } } - contextShadowName = agc.RuntimeCtx.ContextVariableName - if contextShadowName == "nil" { + contextShadowName = agc.RuntimeCtx.NewContextVariableName + if contextShadowName == "nil" || contextShadowName == "" { contextShadowName = amDefaultContextName } @@ -224,6 +224,7 @@ func detectContextIdentImpl(ctx *internal.GeneratorContext, argName string, iden if canonical == vanillaContext && typeName == "Context" { ctx.RuntimeCtx.ContextVariableName = argName + ctx.RuntimeCtx.NewContextVariableName = argName ctx.RuntimeCtx.SpanIDGetter = "" ctx.RuntimeCtx.TraceIDGetter = "" return true, nil @@ -236,6 +237,7 @@ func detectContextIdentImpl(ctx *internal.GeneratorContext, argName string, iden } else { ctx.RuntimeCtx.ContextVariableName = fmt.Sprintf("%s.Context()", argName) } + ctx.RuntimeCtx.NewContextVariableName = "" ctx.RuntimeCtx.SpanIDGetter = "" ctx.RuntimeCtx.TraceIDGetter = "" return true, nil @@ -250,6 +252,7 @@ func detectContextIdentImpl(ctx *internal.GeneratorContext, argName string, iden // Buffalo context embeds a context.Context so it can work like vanilla if canonical == buffalo && typeName == "Context" { ctx.RuntimeCtx.ContextVariableName = argName + ctx.RuntimeCtx.NewContextVariableName = argName ctx.RuntimeCtx.SpanIDGetter = "" ctx.RuntimeCtx.TraceIDGetter = "" return true, nil @@ -275,6 +278,7 @@ func detectContextSelectorImpl(ctx *internal.GeneratorContext, argName string, s for alias, canonical := range ctx.ImportsMap { if canonical == vanillaContext && parentName == alias && typeName == "Context" { ctx.RuntimeCtx.ContextVariableName = argName + ctx.RuntimeCtx.NewContextVariableName = argName ctx.RuntimeCtx.SpanIDGetter = "" ctx.RuntimeCtx.TraceIDGetter = "" return true, nil @@ -288,6 +292,7 @@ func detectContextSelectorImpl(ctx *internal.GeneratorContext, argName string, s } else { ctx.RuntimeCtx.ContextVariableName = fmt.Sprintf("%s.Context()", argName) } + ctx.RuntimeCtx.NewContextVariableName = "" ctx.RuntimeCtx.SpanIDGetter = "" ctx.RuntimeCtx.TraceIDGetter = "" return true, nil @@ -302,6 +307,7 @@ func detectContextSelectorImpl(ctx *internal.GeneratorContext, argName string, s // Buffalo context embeds a context.Context so it can work like vanilla if canonical == buffalo && parentName == alias && typeName == "Context" { ctx.RuntimeCtx.ContextVariableName = argName + ctx.RuntimeCtx.NewContextVariableName = argName ctx.RuntimeCtx.SpanIDGetter = "" ctx.RuntimeCtx.TraceIDGetter = "" return true, nil diff --git a/internal/generate/defer_test.go b/internal/generate/defer_test.go index 6f7d1fa..ff787ec 100644 --- a/internal/generate/defer_test.go +++ b/internal/generate/defer_test.go @@ -216,14 +216,14 @@ func main(w http.ResponseWriter, req *http.Request) { "//\n" + "//autometrics:inst --no-doc --slo \"Service Test\" --success-target 99\n" + "func main(w http.ResponseWriter, req *http.Request) {\n" + - "\treq.Context() = prom.PreInstrument(prom.NewContext(\n" + + "\tamCtx := prom.PreInstrument(prom.NewContext(\n" + "\t\treq.Context(),\n" + "\t\tprom.WithConcurrentCalls(true),\n" + "\t\tprom.WithCallerName(true),\n" + "\t\tprom.WithSloName(\"Service Test\"),\n" + "\t\tprom.WithAlertSuccess(99),\n" + "\t)) //autometrics:shadow-ctx\n" + - "\tdefer prom.Instrument(req.Context(), nil) //autometrics:defer\n" + + "\tdefer prom.Instrument(amCtx, nil) //autometrics:defer\n" + "\n" + " fmt.Println(hello) // line comment 3\n" + "}\n" @@ -274,14 +274,14 @@ func main(w vanilla.ResponseWriter, req *vanilla.Request) { "//\n" + "//autometrics:inst --no-doc --slo \"Service Test\" --success-target 99\n" + "func main(w vanilla.ResponseWriter, req *vanilla.Request) {\n" + - "\treq.Context() = prom.PreInstrument(prom.NewContext(\n" + + "\tamCtx := prom.PreInstrument(prom.NewContext(\n" + "\t\treq.Context(),\n" + "\t\tprom.WithConcurrentCalls(true),\n" + "\t\tprom.WithCallerName(true),\n" + "\t\tprom.WithSloName(\"Service Test\"),\n" + "\t\tprom.WithAlertSuccess(99),\n" + "\t)) //autometrics:shadow-ctx\n" + - "\tdefer prom.Instrument(req.Context(), nil) //autometrics:defer\n" + + "\tdefer prom.Instrument(amCtx, nil) //autometrics:defer\n" + "\n" + " fmt.Println(hello) // line comment 3\n" + "}\n" @@ -332,14 +332,14 @@ func main(w ResponseWriter, req *Request) { "//\n" + "//autometrics:inst --no-doc --slo \"Service Test\" --success-target 99\n" + "func main(w ResponseWriter, req *Request) {\n" + - "\treq.Context() = prom.PreInstrument(prom.NewContext(\n" + + "\tamCtx := prom.PreInstrument(prom.NewContext(\n" + "\t\treq.Context(),\n" + "\t\tprom.WithConcurrentCalls(true),\n" + "\t\tprom.WithCallerName(true),\n" + "\t\tprom.WithSloName(\"Service Test\"),\n" + "\t\tprom.WithAlertSuccess(99),\n" + "\t)) //autometrics:shadow-ctx\n" + - "\tdefer prom.Instrument(req.Context(), nil) //autometrics:defer\n" + + "\tdefer prom.Instrument(amCtx, nil) //autometrics:defer\n" + "\n" + " fmt.Println(hello) // line comment 3\n" + "}\n"