contrib/net/http: refactor tracing #2921

rarguelloF · 2024-10-09T17:41:38Z

What does this PR do?

Refactor contrib/net/http.TraceAndServe to extract the logic ran before executing the provided handler into a separate function: contrib/internal/httptrace.BeforeHandle.
BeforeHandle returns the modified ResponseWriter and Request types with the required tracing information included. It also provides an afterHandle function that should be executed after the Handler runs, and a handled bool that instructs the original handler should not be called if true (this was required for the appsec logic).
Move around some of the contrib/net/http types to contrib/internal/httptrace (motivated by the changes described above), adding type aliases in contrib/net/http for the exported ones.
Similar changes described above for contrib/julienschmidt/httprouter, adding a new internal/tracing package with a BeforeHandle function that uses generics to avoid importing the original package (and therefore be reusable in orchestrion).
Refactor in internal/appsec/emitter/httpsec motivated by the refactor described above.

Motivation

Orchestrion support.

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
There is a benchmark for any new code, or changes to existing code.
If this interacts with the agent in a new way, a system test has been added.
Add an appropriate team label so this PR gets put in the right place for the release notes.
Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

pr-commenter · 2024-10-09T18:32:35Z

Benchmarks

Benchmark execution time: 2024-10-15 11:23:28

Comparing candidate commit cbcce81 in PR branch rarguelloF/APPSEC-55047/refactor-trace-http with baseline commit 93311db in branch main.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 57 metrics, 1 unstable metrics.

scenario:BenchmarkHttpServeTrace-24

🟥 allocations [+2; +2] or [+2.597%; +2.597%]

RomainMuller · 2024-10-14T09:34:58Z

contrib/internal/httptrace/response_writer.go

+ if w.status != 0 {
+ return
+ }
+ w.ResponseWriter.WriteHeader(status)
+ w.status = status


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:

Suggested change

if w.status != 0 {

return

}

w.ResponseWriter.WriteHeader(status)

w.status = status

w.ResponseWriter.WriteHeader(status)

if w.status == 0 {

w.status = status

}

RomainMuller · 2024-10-14T09:39:20Z

contrib/internal/httptrace/before_handle.go

+ SpanOpts []ddtrace.StartSpanOption
+}
+
+func BeforeHandle(cfg *ServeConfig, w http.ResponseWriter, r *http.Request) (http.ResponseWriter, *http.Request, func(), bool) {


Strongly suggest adding documentation comments here, at least describing what the results are for (especially the last two, as they have "opaque" types & are un-named).

RomainMuller · 2024-10-14T09:40:55Z

contrib/julienschmidt/httprouter/internal/tracing/config.go

+ serviceName string
+ spanOpts []ddtrace.StartSpanOption
+ analyticsRate float64
+ headerTags *internal.LockMap


Suggest front-loading pointers to make the GC's life easier

Suggested change

serviceName string

spanOpts []ddtrace.StartSpanOption

analyticsRate float64

headerTags *internal.LockMap

headerTags *internal.LockMap

spanOpts []ddtrace.StartSpanOption

serviceName string

analyticsRate float64

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.

RomainMuller · 2024-10-14T09:42:39Z

contrib/julienschmidt/httprouter/internal/tracing/tracing.go

+ 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) {


Same as before, this could use documentation commentary

RomainMuller · 2024-10-14T09:44:18Z

contrib/julienschmidt/httprouter/internal/tracing/tracing.go

+ for _, param := range ps {
+ route = strings.Replace(route, param.GetValue(), ":"+param.GetKey(), 1)
+ }


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?)

nice catch, I'll open a subsequent PR adding some extra test-cases and try to do this replacement in a smarter way.

RomainMuller · 2024-10-14T09:47:10Z

internal/appsec/emitter/httpsec/http.go

+func BeforeHandle(w http.ResponseWriter, r *http.Request, span ddtrace.Span, pathParams map[string]string, opts *Config) (
+ http.ResponseWriter, *http.Request, func(), bool) {


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):

Suggested change

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) {

yeah agree this looks much better

RomainMuller · 2024-10-14T09:49:10Z

internal/appsec/emitter/httpsec/http.go

+ if res, ok := w.(interface{ Status() int }); ok {
+ statusCode = res.Status()
+ }


Consider adding an else that prints a debug statement indicating the writer somehow misses the Status() method (+ mention its actual type).

Suggested change

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)

}

RomainMuller · 2024-10-14T09:51:34Z

internal/appsec/emitter/httpsec/http.go

+ blockPtr.Handler.ServeHTTP(w, tr)
+ blockPtr.Handler = nil


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?

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

contrib/net/http: refactor tracing

87ab562

github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Oct 9, 2024

rarguelloF added 3 commits October 9, 2024 15:13

refactor

61beec7

add appsec

6f6d7d6

add copyright header

55426a1

RomainMuller reviewed Oct 14, 2024

View reviewed changes

rarguelloF added 2 commits October 14, 2024 13:36

review comments

5a0c9bb

Merge branch 'main' into rarguelloF/APPSEC-55047/refactor-trace-http

cbcce81

rarguelloF marked this pull request as ready for review October 16, 2024 08:44

rarguelloF requested review from a team as code owners October 16, 2024 08:44

rarguelloF changed the title ~~WIP: contrib/net/http: refactor tracing~~ contrib/net/http: refactor tracing Oct 16, 2024

eliottness approved these changes Oct 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/net/http: refactor tracing #2921

contrib/net/http: refactor tracing #2921

rarguelloF commented Oct 9, 2024 •

edited

Loading

pr-commenter bot commented Oct 9, 2024 •

edited

Loading

RomainMuller Oct 14, 2024

RomainMuller Oct 14, 2024

RomainMuller Oct 14, 2024

rarguelloF Oct 14, 2024

RomainMuller Oct 14, 2024

RomainMuller Oct 14, 2024

rarguelloF Oct 14, 2024 •

edited

Loading

RomainMuller Oct 14, 2024

rarguelloF Oct 14, 2024

RomainMuller Oct 14, 2024

RomainMuller Oct 14, 2024

eliottness Oct 14, 2024

		func BeforeHandle(w http.ResponseWriter, r http.Request, span ddtrace.Span, pathParams map[string]string, opts Config) (
		http.ResponseWriter, *http.Request, func(), bool) {

contrib/net/http: refactor tracing #2921

Are you sure you want to change the base?

contrib/net/http: refactor tracing #2921

Conversation

rarguelloF commented Oct 9, 2024 • edited Loading

What does this PR do?

Motivation

Reviewer's Checklist

pr-commenter bot commented Oct 9, 2024 • edited Loading

Benchmarks

scenario:BenchmarkHttpServeTrace-24

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rarguelloF Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rarguelloF commented Oct 9, 2024 •

edited

Loading

pr-commenter bot commented Oct 9, 2024 •

edited

Loading

rarguelloF Oct 14, 2024 •

edited

Loading