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

Fix debug pprof handler routing #446

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

jace-ys
Copy link
Contributor

@jace-ys jace-ys commented Aug 19, 2024

Hey folks 👋🏻 Just started using goa.design and I really like it so far!

While using the clue debug handlers, I noticed that the pprof profile handlers like goroutine and mutex, other than cmdline, profile, symbol and trace, were not actually working and returned 404.

After some digging and testing, it seems that's because those profile handlers are not explicitly mounted on the mux. While this works for http.NewServeMux (like in debug_test.go), it doesn't actually work with chi.Mux or even gorilla mux.Router - not entirely sure why, but I'd bet it's something to do with how routing decisions are implemented in the stdlib vs. third-party libraries.

You can see that here in this test:

type ChiMuxWrapper struct {
	*chi.Mux
}

func (w *ChiMuxWrapper) HandleFunc(pattern string, handler func(http.ResponseWriter, *http.Request)) {
	w.Mux.HandleFunc(pattern, handler)
}

func TestMountPprofHandlers(t *testing.T) {
	mux := chi.NewRouter()
	MountPprofHandlers(&ChiMuxWrapper{mux})
	MountPprofHandlers(&ChiMuxWrapper{mux}, WithPrefix("test"))
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.URL.Path != "/" {
			w.WriteHeader(http.StatusNotFound)
			return
		}
		w.WriteHeader(http.StatusOK)
		w.Write([]byte("OK")) // nolint: errcheck
	})
	mux.Handle("/", handler)
	ts := httptest.NewServer(mux)
	defer ts.Close()

	status, resp := makeRequest(t, ts.URL)
	if status != http.StatusOK {
		t.Errorf("got status %d, expected %d", status, http.StatusOK)
	}
	if resp != "OK" {
		t.Errorf("got body %q, expected %q", resp, "OK")
	}

	paths := []string{
		"/debug/pprof/",
		"/test/",
		"/debug/pprof/allocs",
		"/debug/pprof/block",
		"/debug/pprof/cmdline",
		"/debug/pprof/goroutine",
		"/debug/pprof/heap",
		"/debug/pprof/mutex",
		// "/debug/pprof/profile?seconds=1", # Takes too long to run on each test
		"/debug/pprof/symbol",
		"/debug/pprof/threadcreate",
		// "/debug/pprof/trace", # Takes too long to run on each test
	}
	for _, path := range paths {
		status, resp = makeRequest(t, ts.URL+path)
		if status != http.StatusOK {
			t.Errorf("got status %d, expected %d", status, http.StatusOK)
		}
		if resp == "" {
			t.Errorf("got body %q, expected non-empty", resp)
		}
	}
}

I've proposed a fix for this by explicitly mounting each pprof handler, similar to what chi's middleware.Profiler does: https://github.com/go-chi/chi/blob/master/middleware/profiler.go

Let me know what you think!

@raphael
Copy link
Member

raphael commented Aug 19, 2024

Thank you for raising this issue and the great PR. It all makes sense. I've fixed the lint issue introduced by upgrading the linter tool in the main branch. Would you mind rebasing your PR on top? Thank you!

@jace-ys jace-ys force-pushed the jace/fix-debug-pprof-routing branch from 356a6f9 to 7ba93a3 Compare August 19, 2024 21:49
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.79%. Comparing base (59e3094) to head (7ba93a3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
+ Coverage   85.74%   85.79%   +0.04%     
==========================================
  Files          36       36              
  Lines        1747     1753       +6     
==========================================
+ Hits         1498     1504       +6     
  Misses        225      225              
  Partials       24       24              
Flag Coverage Δ
micro 85.79% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jace-ys
Copy link
Contributor Author

jace-ys commented Aug 20, 2024

Thank you for raising this issue and the great PR. It all makes sense. I've fixed the lint issue introduced by upgrading the linter tool in the main branch. Would you mind rebasing your PR on top? Thank you!

Done!

@raphael
Copy link
Member

raphael commented Aug 21, 2024

Thank you!

@raphael raphael merged commit 8ae47a5 into goadesign:main Aug 21, 2024
4 checks passed
@jace-ys jace-ys deleted the jace/fix-debug-pprof-routing branch August 21, 2024 16:35
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.

2 participants