servenv: Move away from using default HTTP muxer #12987
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dbussink
added
Type: Internal Cleanup
Component: General
vitess-bot
bot
added
NeedsDescriptionUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
If a new flag is being introduced:
If a workflow is added or modified:
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
dbussink
removed
NeedsDescriptionUpdate
dbussink
force-pushed
the
remove-default-mux-usage
branch
3 times, most recently
from
c7bdb92 to
5480dde
Compare
Up until now we've been using the implicit default HTTP mux. The problem of using this mux is that it works implicitly also for profiling endpoints. So the moment that `net/http/pprof` is imported, those endpoints get added. We want to be able to make enabling the profiling endpoints optional, but there's no way to do that with the default mux except by recompiling and removing the import. By moving everything away from the default mux to an explicit mux for the servenv, we can make this configurable in the near future. It still gets added to the default mux, but we don't use that anymore so it doesn't get exposed. The changes here now explicitly add the profiling endpoints so that we can make that optional in a follow up. Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
force-pushed
the
remove-default-mux-usage
branch
from
5480dde to
94fbd24
Compare
dbussink
requested review from
harshit-gangal,
systay,
shlomi-noach,
rohit-nayak-ps,
mattlord,
deepthi,
GuptaManan100,
rsajwani,
frouioui,
ajm188 and
notfelineit
as code owners
ajm188
reviewed
Apr 27, 2023
Comment on lines
-32
to
-33
| // register the HTTP handlers for profiling | ||
| _ "net/http/pprof" |
Because of the http pprof import that exists today and our usage of the default mux, we always end up with the http profiling endpoints today. It’s not possible to turn those off and on because of that. Anyone using Vitess could be using those endpoints to enable / disable specific profilers at runtime through those pprof http endpoints. We use these for example in PlanetScale internally when we need to debug issues with Vitess to do things like dump stack traces for goroutines. That’s separate from the profiling flags which end up always enabling the profiling depending on how those flags are given. So they are separate things today. The net change of the refactor here is that for the end user everything still works the same. |
Signed-off-by: Dirkjan Bussink <[email protected]>
Signed-off-by: Dirkjan Bussink <[email protected]>
4 tasks
ajm188
approved these changes
Apr 27, 2023
dbussink
added a commit
to planetscale/vitess
that referenced
this pull request
Jul 27, 2023
Since vitessio#12987 profiling is not loaded implicitly through an import but only explicitly. This means we need to make sure that `servenv.Init` is called. It's not called for all binaries which is an oversight. It also explains why this wasn't found with vitessio#12987 since that was tested with binaries like `vttablet` and `vtgate` and it was assumed that this loading was done everywhere properly, which it sadly was not. Signed-off-by: Dirkjan Bussink <[email protected]>
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Up until now we've been using the implicit default HTTP mux. The problem of using this mux is that it works implicitly also for profiling endpoints. So the moment that
net/http/pprofis imported, those endpoints get added.We want to be able to make enabling the profiling endpoints optional, but there's no way to do that with the default mux except by recompiling and removing the import.
By moving everything away from the default mux to an explicit mux for the servenv, we can make this configurable in the near future. It still gets added to the default mux, but we don't use that anymore so it doesn't get exposed.
The changes here now explicitly add the profiling endpoints so that we can make that optional in a follow up.
Checklist