contrib/net/http: refactor tracing #2921
Conversation
github-actions
bot
added
the
apm:ecosystem
BenchmarksBenchmark execution time: 2024-10-15 11:23:28 Comparing candidate commit cbcce81 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 57 metrics, 1 unstable metrics. scenario:BenchmarkHttpServeTrace-24
|
| if w.status != 0 { | ||
| return | ||
| } | ||
| w.ResponseWriter.WriteHeader(status) | ||
| w.status = status |
There was a problem hiding this comment.
The "regular" ResponseWriter implementation has some logging code when it's called multiple times; and I feel like this conditional might break this semantics...
I think I'd probably change this to:
| if w.status != 0 { | |
| return | |
| } | |
| w.ResponseWriter.WriteHeader(status) | |
| w.status = status | |
| w.ResponseWriter.WriteHeader(status) | |
| if w.status == 0 { | |
| w.status = status | |
| } |
| serviceName string | ||
| spanOpts []ddtrace.StartSpanOption | ||
| analyticsRate float64 | ||
| headerTags *internal.LockMap |
There was a problem hiding this comment.
Suggest front-loading pointers to make the GC's life easier
| serviceName string | |
| spanOpts []ddtrace.StartSpanOption | |
| analyticsRate float64 | |
| headerTags *internal.LockMap | |
| headerTags *internal.LockMap | |
| spanOpts []ddtrace.StartSpanOption | |
| serviceName string | |
| analyticsRate float64 |
There was a problem hiding this comment.
thanks! even though this is a "just" a refactor PR and I wanted to avoid extra changes, since this is a pretty-safe to make change I will just update here.
| GetValue() string | ||
| } | ||
|
|
||
| func BeforeHandle[T any, WT Router](cfg *Config, router T, wrapRouter func(T) WT, w http.ResponseWriter, req *http.Request) (http.ResponseWriter, *http.Request, func(), bool) { |
There was a problem hiding this comment.
Same as before, this could use documentation commentary
| for _, param := range ps { | ||
| route = strings.Replace(route, param.GetValue(), ":"+param.GetKey(), 1) | ||
| } |
There was a problem hiding this comment.
FWIW, this could replace the wrong part of the URI, if a parameter's value is actually a substring of any other segment of the URI. I'm also not sure how the contraption behaves in presence of URL-encoded characters (will the param.GetValue() return the un-escaped version?)
There was a problem hiding this comment.
nice catch, I'll open a subsequent PR adding some extra test-cases and try to do this replacement in a smarter way.
| func BeforeHandle(w http.ResponseWriter, r *http.Request, span ddtrace.Span, pathParams map[string]string, opts *Config) ( | ||
| http.ResponseWriter, *http.Request, func(), bool) { |
There was a problem hiding this comment.
Same as before, documentation would be welcome.
Also, I'm not a huge fan of the line break after the results parenthesis... I'm a little grossed out... For these I prefer the following (which I reckon gofmt isn't against):
| func BeforeHandle(w http.ResponseWriter, r *http.Request, span ddtrace.Span, pathParams map[string]string, opts *Config) ( | |
| http.ResponseWriter, *http.Request, func(), bool) { | |
| func BeforeHandle( | |
| w http.ResponseWriter, | |
| r *http.Request, | |
| span ddtrace.Span, | |
| pathParams map[string]string, | |
| opts *Config, | |
| ) (http.ResponseWriter, *http.Request, func(), bool) { |
There was a problem hiding this comment.
yeah agree this looks much better
| if res, ok := w.(interface{ Status() int }); ok { | ||
| statusCode = res.Status() | ||
| } |
There was a problem hiding this comment.
Consider adding an else that prints a debug statement indicating the writer somehow misses the Status() method (+ mention its actual type).
| if res, ok := w.(interface{ Status() int }); ok { | |
| statusCode = res.Status() | |
| } | |
| if res, ok := w.(interface{ Status() int }); ok { | |
| statusCode = res.Status() | |
| } else { | |
| log.Debugf("http.ResponseWriter (%T) lacks Statsus()int method", w) | |
| } |
| blockPtr.Handler.ServeHTTP(w, tr) | ||
| blockPtr.Handler = nil |
There was a problem hiding this comment.
This is making me feel a little unsafe (@eliottness can you have a look?).
Basically, the blockPtr comes from an atomic pointer, suggesting it may be concurrently modified... And then we're changing one value in there that was checked above... so we have a possible race condition here...
We may need to also use an atomic pointer here, and atomic-swap it with a nil?
There was a problem hiding this comment.
Took note of the issues you mention in appsec code. Although the atomic pointer was us being overzealous in the first place, let's be consistent and replace the atomic.Load() calls to atomic.CompareAndSwap in another PR so we get rid of this race
rarguelloF
changed the title
Co-authored-by: Dario Castañé <[email protected]>
Adds support for `github.com/julienschmidt/httprouter` Requires DataDog/dd-trace-go#2921
What does this PR do?
contrib/net/http.TraceAndServeto extract the logic ran before executing the provided handler into a separate function:contrib/internal/httptrace.BeforeHandle.BeforeHandlereturns the modifiedResponseWriterandRequesttypes with the required tracing information included. It also provides anafterHandlefunction that should be executed after theHandlerruns, and ahandledbool that instructs the original handler should not be called if true (this was required for the appsec logic).contrib/net/httptypes tocontrib/internal/httptrace(motivated by the changes described above), adding type aliases incontrib/net/httpfor the exported ones.contrib/julienschmidt/httprouter, adding a newinternal/tracingpackage with aBeforeHandlefunction that uses generics to avoid importing the original package (and therefore be reusable in orchestrion).internal/appsec/emitter/httpsecmotivated by the refactor described above.Motivation
Orchestrion support.
Reviewer's Checklist
Unsure? Have a question? Request a review!