Skip to content
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] add option to wrap a provided *http.ServeMux #2690

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

powerbill
Copy link

There are many cases where the traced serve mux should be the 'outer' most http handler in a chain of wrapped handlers. For example, if one wanted to write a middleware which logged all requests by wrapping a http.Handler and they wanted this logger to log dd.trace_id and dd.span_id, that logger would have to execute after the traced http.Handler. One could achieve this using the
contrib/net/http#WrapHandler function, but this loses the nice capability of naming the resource using the http.ServeMux#Handler method because the resourceNamer config does not have access to the mux itself.

Another approach, would be to have the contrib/net/http.ServeMux take another ServeMux as an option, and use that as its mux if provided. This way, the first 'handler' in a chain of request handlers would be the contrib/net/http.ServeMux. This traced mux would add trace context to the http request. This traced request would then be passed to the e.g. logging middleare mux, and so on, and so forth.

What does this PR do?

Motivation

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!

There are many cases where the traced serve mux should be the 'outer'
most http handler in a chain of wrapped handlers. For example, if
one wanted to write a middleware which logged all requests by wrapping
a http.Handler and they wanted this logger to log dd.trace_id and
dd.span_id, that logger would have to execute after the traced
http.Handler. One could achieve this using the
contrib/net/http#WrapHandler function, but this loses the nice
capability of naming the resource using the http.ServeMux#Handler method
because the resourceNamer config does not have access to the mux itself.

Another approach, would be to have the contrib/net/http.ServeMux take
another ServeMux as an option, and use that as its mux if provided. This
way, the first 'handler' in a chain of request handlers would be the
contrib/net/http.ServeMux. This traced mux would add trace context to
the http request. This traced request would then be passed to the e.g.
logging middleare mux, and so on, and so forth.
@powerbill powerbill requested review from a team as code owners May 10, 2024 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant