contrib/net/http: support for path params from go 1.22 proposal#3079
Conversation
eliottness
added
apm:ecosystem
Signed-off-by: Eliott Bouhana <[email protected]>
deb7177 to
9727b94
Compare
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
BenchmarksBenchmark execution time: 2025-01-14 17:24:34 Comparing candidate commit 697c5c0 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 58 metrics, 0 unstable metrics. scenario:BenchmarkHttpServeTrace-24
|
rarguelloF
left a comment
rarguelloF
left a comment
There was a problem hiding this comment.
LGTM! just left a small nit to improve readability 🙇
| // Wildcard names in a path must be distinct. | ||
| func patternNames(pattern string) ([]string, error) { |
There was a problem hiding this comment.
nit: I had to dig into the implementation and tests to understand what this function returns (since the type []string doesn't give much information). I think it would be nice if this was added to the doc string for future readers to quickly understand what this function does.
Adding something at the end like:
Given a pattern like "GET /foo/{bar}/{baz}", it returns []string{"bar", "baz"}
Also having 2 functions getPatternNames and patternNames with almost the same function signature doesn't help much (the difference is the first one uses the cache first). WDYT about renaming this one to parsePattern or parsePatternNames? (ideally I think it should be parsePattern, because if we want to parse other stuff from it in the future we would likely want to do it only once).
There was a problem hiding this comment.
Applied all your comments (I believe) 👍
Signed-off-by: Eliott Bouhana <[email protected]>
…lloF Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
3 participants
What does this PR do?
This PR add support for importing path params into
dd-trace-go/contrib/internal/httptracedata pipeline as multiple other contribs already do.In details I did:
net/httpinto dd-trace-go(*http.Request).PathValue(name)and build amap[string]stringwith itMotivation
Reviewer's Checklist
v2-devbranch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!