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

WIP: instrument net/http.ServeHTTP implementations #175

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

Conversation

nsrip-dd
Copy link
Contributor

@nsrip-dd nsrip-dd commented Jul 19, 2024

What does this PR do?

TODO - tests

Makes sure we instrument net/http handlers and servers.

Background

There are two general ways to start an HTTP server with net/http:

  • Package-level functions:
    • http.ListenAndServe
    • http.ListenAndServeTLS
    • http.Serve
    • http.ServeTLS
  • Via the net/http.Server methods:
    • http.(*Server).ListenAndServe
    • http.(*Server).ListenAndServeTLS
    • http.(*Server).Serve
    • http.(*Server).ServeTLS

The package-level functions create a Server and dispatch to the matching methods.

All of these work with the http.Handler interface, which defines the "application" logic for handling an HTTP request. The Server type has a Handler field, and all of the package-level functions take an http.Handler argument. Many packages, like github.com/gorilla/mux, provide http.Handler implementations.

The net/http package has two http.Handler implementations: net/http.ServeMux, and net/http.HandlerFunc.

What this PR actually does

Before, we instrumented the Handler field of Server literals, and function literals cast to http.HandlerFunc. But this could lead to redundant instrumentation, and also didn't cover common patterns. For example, this doesn't get instrumented today:

package main

import "net/http"

type Handler struct{}

func (Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {}

func main() {
	mu := http.NewServeMux()
	mu.Handle("/foo", Handler{})
	http.ListenAndServe(":8080", mu)
}

For another example, we get redundant instrumentation for this github.com/gorilla/mux example:

mux := mux.NewRouter()
tc.Server = &http.Server{
	Addr:    "127.0.0.1:" + utils.GetFreePort(t),
	Handler: mux,
}

mux.HandleFunc("/ping", func(w http.ResponseWriter, _ *http.Request) {
	w.Header().Set("Content-Type", "application/json")
	w.WriteHeader(http.StatusOK)
	_, err := io.WriteString(w, `{"message": "pong"}`)
	assert.NoError(t, err)
}).Methods("GET")

The Handler field of the Server literal gets instrumented, and the http.HandlerFunc literal is instrumented, and the router returned via mux.NewRouter is instrumented. We have at least one too many layers of instrumentation.

So this PR takes this approach:

  • We define a new MaybeWrapHandler function. It takes an http.Handler and returns an http.Handler. If the input handler is one of the net/http handler types, we wrap it with tracing instrumentation. Otherwise we pass it on unchanged under the assumption it is already instrumented.
  • We use this function to maybe wrap the http.Handler passed to the package-level functions.
  • We also use this function to maybe wrap the Handler field of a Server literal, similar to what we did before.

This doesn't provide complete coverage. For example, the user could do something like s := new(http.Server); s.Handler = foo and we wouldn't catch it. But this hopefully improves our coverage significantly and avoids some redundant instrumentation.

Motivation

Fixes #173

Reviewer's Checklist

  • Changed code has unit tests for its functionality.

@nsrip-dd nsrip-dd force-pushed the nick.ripley/instrument-servehttp branch 2 times, most recently from 47e5482 to ac2604c Compare July 23, 2024 23:27
@nsrip-dd nsrip-dd force-pushed the nick.ripley/instrument-servehttp branch from ac2604c to 71230cd Compare October 11, 2024 07:47
Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 56.48%. Comparing base (0acfd19) to head (71230cd).

Files with missing lines Patch % Lines
instrument/http.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
- Coverage   58.16%   56.48%   -1.68%     
==========================================
  Files         151      105      -46     
  Lines       10976     5051    -5925     
==========================================
- Hits         6384     2853    -3531     
+ Misses       4124     1873    -2251     
+ Partials      468      325     -143     
Components Coverage Δ
Generators 78.38% <ø> (+1.40%) ⬆️
Instruments 61.33% <0.00%> (-26.73%) ⬇️
Go Driver 48.55% <ø> (-24.26%) ⬇️
Toolexec Driver 42.84% <ø> (-28.05%) ⬇️
Aspects 71.34% <ø> (+0.49%) ⬆️
Injector 72.71% <ø> (-0.44%) ⬇️
Job Server 54.18% <ø> (-9.85%) ⬇️
Integration Test Suite 38.68% <ø> (-9.81%) ⬇️
Other 56.48% <0.00%> (-1.68%) ⬇️
Files with missing lines Coverage Δ
instrument/http.go 0.00% <0.00%> (-100.00%) ⬇️

... and 136 files with indirect coverage changes

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.

net/http: Handler interface implementation is not always instrumented
1 participant