-
Notifications
You must be signed in to change notification settings - Fork 441
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
contrib/net/http: support for path params from go 1.22 proposal #3079
Conversation
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! just left a small nit to improve readability 🙇
contrib/net/http/pattern.go
Outdated
// Wildcard names in a path must be distinct. | ||
func patternNames(pattern string) ([]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
What does this PR do?
This PR add support for importing path params into
dd-trace-go/contrib/internal/httptrace
data pipeline as multiple other contribs already do.In details I did:
net/http
into dd-trace-go(*http.Request).PathValue(name)
and build amap[string]string
with itMotivation
Reviewer's Checklist
v2-dev
branch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!