fix(contrib/net/http): cannot use http.Request.PathValue before http.ServeMux.ServeHTTP is called by eliottness · Pull Request #3853 · DataDog/dd-trace-go

eliottness · 2025-08-06T13:16:10Z

Motivation

A system-test using path params to check for security event has been failing for a longtime already and we recently discovered that is was linked to the way the net-http system-tests weblog is done: using contrib/net/http.NewServeMux instead of contrib/net/http.WrapHandler. Both instrumentation works except one is before the routing of the router and one is after. Being before was okay before Go 1.22 introduced the pattern system that get's filled in the request from inside the call to http.ServeMux.ServeHTTP of which we have one instrumentation mode being before and one after. This means that calls to r.PathValue and access to r.Pattern always return a blank string before this happen even if the request object already exist

This means that we need to move part of appsec processing always after the routing process happened. This PR does exactly this by taking advantage of the fact that when there is no data sent, the empty values sent earlier would be ignored later in the pipeline anyway.

What does this PR do?

Move the pattern package out of instrumentation like it should be
Remove the getPattern function that is an artifact of when there was two versions of it before Go 1.22
Create an httpsec.RouteMatched function that does the job of filling the gap left by not having access to route and path params
wrap the ServeMux.Handle and ServeMux.HandleFunc to call httpsec.RouteMatched when necessary.

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.
New code is free of linting errors. You can check this by running ./scripts/lint.sh locally.
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!

…HTTP is called Signed-off-by: Eliott Bouhana <[email protected]>

pr-commenter · 2025-08-06T13:22:36Z

Benchmarks

Benchmark execution time: 2025-08-06 15:51:00

Comparing candidate commit 7c8b2d9 in PR branch eliottness/APPSEC-57080/path-params-net-http-serve-mux with baseline commit 455bd92 in branch main.

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

datadog-official · 2025-08-06T13:24:12Z

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 7c8b2d9 | Docs | Was this helpful? Give us feedback!}

Signed-off-by: Eliott Bouhana <[email protected]>

RomainMuller

LGTM

Signed-off-by: Eliott Bouhana <[email protected]>

darccio · 2025-08-07T07:55:33Z

 			// Ignore the error: Something as gone wrong, but we are not eager to find out why.
 			// We will just log it as a telemetry logs warning (and Debug to the user-facing log).
-			log.Warn("instrumentation/net/http/pattern: failed to parse mux path pattern %q: %s", pattern, err.Error())
+			internal.Instrumentation.Logger().Warn("instrumentation/net/http/pattern: failed to parse mux path pattern %q: %s", pattern, err.Error())


I'm curious about why this file was moved to the internal package for net/http. It doesn't seem to break anything, so I'm fine with it. Is it for the sake of reducing the scope of the instrumentation package?

Scope-wise this does not make sense for the pattern package to be alone in instrumentation while all other code related to net/http is in the contrib. Considering the PR doing this is one of the last we did on v1 I would say this is an artifact of how v1 was done and how this package was migrated last minute.

The instrumentation/net/http/pattern package was moved by #3853. The contribu/net/http/v2 module depends on that package at version v2.2.3. As a result, that verison of contrib/net/http/v2 won't be able to use newer versions of the main module. This is a breaking change. This commit restores the deleted package. While the current contrib/net/http/v2 package doesn't use it any more, this maintains compatibility with previous versions.

The instrumentation/net/http/pattern package was moved by #3853. The contrib/net/http/v2 module depends on that package at version v2.2.3. As a result, that verison of contrib/net/http/v2 won't be able to use newer versions of the main module. This is a breaking change. This commit restores the deleted package. While the current contrib/net/http/v2 package doesn't use it any more, this maintains compatibility with previous versions.

…backport) The instrumentation/net/http/pattern package was moved by #3853. The contrib/net/http/v2 module depends on that package at version v2.2.3. As a result, that verison of contrib/net/http/v2 won't be able to use newer versions of the main module. This is a breaking change. This commit restores the deleted package. While the current contrib/net/http/v2 package doesn't use it any more, this maintains compatibility with previous versions. In this backport, we don't have the NewSafeError functionality for telemetry logging. Rather than bring it back to this branch, this commit just uses the `%T` formatter to get roughly the same functionality. Backports #4017

fix(contrib/net/http): cannot use http.Request.PathValue before Serve…

332ab93

…HTTP is called Signed-off-by: Eliott Bouhana <[email protected]>

github-actions Bot added the apm:ecosystem contrib/* related feature requests or bugs label Aug 6, 2025

eliottness added 4 commits August 6, 2025 17:00

rework mux Handle and HandlerFunc to sent standalone data to appsec

d290c22

Signed-off-by: Eliott Bouhana <[email protected]>

fix build error

b7d0e18

Signed-off-by: Eliott Bouhana <[email protected]>

comments

9b0c919

Signed-off-by: Eliott Bouhana <[email protected]>

fix missing call to ServeHTTP

3366791

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness marked this pull request as ready for review August 6, 2025 15:27

eliottness requested review from a team as code owners August 6, 2025 15:27

eliottness added the appsec label Aug 6, 2025

RomainMuller approved these changes Aug 6, 2025

View reviewed changes

revert handler.go

7c8b2d9

Signed-off-by: Eliott Bouhana <[email protected]>

darccio approved these changes Aug 7, 2025

View reviewed changes

rarguelloF approved these changes Aug 7, 2025

View reviewed changes

eliottness merged commit 3aac4d6 into main Aug 7, 2025
244 checks passed

eliottness deleted the eliottness/APPSEC-57080/path-params-net-http-serve-mux branch August 7, 2025 08:53

eliottness mentioned this pull request Aug 25, 2025

feat(appsec): blocking on path params for net/http serve mux tracing #3914

Merged

7 tasks

nsrip-dd mentioned this pull request Oct 2, 2025

fix(instrumentation): restore deleted net/http/pattern package #4017

Merged

nsrip-dd mentioned this pull request Oct 2, 2025

fix(instrumentation): restore deleted net/http/pattern package (#4017 backport) #4019

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(contrib/net/http): cannot use http.Request.PathValue before http.ServeMux.ServeHTTP is called#3853

fix(contrib/net/http): cannot use http.Request.PathValue before http.ServeMux.ServeHTTP is called#3853
eliottness merged 6 commits into
mainfrom
eliottness/APPSEC-57080/path-params-net-http-serve-mux

eliottness commented Aug 6, 2025 •

edited

Loading

Uh oh!

pr-commenter Bot commented Aug 6, 2025 •

edited

Loading

Uh oh!

datadog-official Bot commented Aug 6, 2025 •

edited

Loading

Uh oh!

RomainMuller left a comment

Uh oh!

darccio Aug 7, 2025

Uh oh!

eliottness Aug 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

eliottness commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

What does this PR do?

Reviewer's Checklist

Uh oh!

pr-commenter Bot commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

datadog-official Bot commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RomainMuller left a comment

Choose a reason for hiding this comment

Uh oh!

darccio Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

eliottness Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eliottness commented Aug 6, 2025 •

edited

Loading

pr-commenter Bot commented Aug 6, 2025 •

edited

Loading

datadog-official Bot commented Aug 6, 2025 •

edited

Loading