-
Notifications
You must be signed in to change notification settings - Fork 614
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
Use /stats/summary for metrics handler #853
Conversation
…09f4e24bb879404e2add887e106e472d8b
How about we add the metrics routes to the PodHandler? |
Sure, that's actually where it belongs. We'll have to thread the changes through node-cli, though. Let me refactor this to move the stats route into PodHandlerConfig as a first step. |
@cpuguy83 can you take another look now? |
node/api/server.go
Outdated
// | ||
// If the passed in handler func is nil this will create handlers which only | ||
// serves http.StatusNotImplemented | ||
func PodStatsSummaryHandler(f PodStatsSummaryHandlerFunc) http.Handler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not delete existing functions.
We can keep the old one for attaching to other
node/api/server.go
Outdated
@@ -111,7 +98,7 @@ type PodMetricsConfig struct { | |||
// Callers should take care to namespace the serve mux as they see fit, however | |||
// these routes get called by the Kubernetes API server. | |||
func AttachPodMetricsRoutes(p PodMetricsConfig, mux ServeMux) { | |||
mux.Handle("/", InstrumentHandler(HandlePodStatsSummary(p.GetStatsSummary))) | |||
mux.Handle("/stats/summary", InstrumentHandler(HandlePodStatsSummary(p.GetStatsSummary))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should remain as is.
If both the metrics routes and the pod routes are attached to the same mux with the pattern "/", it will panic. Instead, add the stats handler function to PodHandlerConfig and set up the route if it is not nil.
@@ -64,6 +65,13 @@ func PodHandler(p PodHandlerConfig, debug bool) http.Handler { | |||
WithExecStreamIdleTimeout(p.StreamIdleTimeout), | |||
), | |||
).Methods("POST", "GET") | |||
|
|||
if p.GetStatsSummary != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, HandlePotStatsSummary
will return a NotImplemented
handler if this is nil anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
To allow users to attach both the pod and metrics handlers to the same mux, use "/stats/summary" as the pattern in AttachPodMetricsRoutes. Otherwise, if mux.Handle() is called again for the same pattern, it will panic, and this is what happens in node-cli due to virtual-kubelet/node-cli#27 calling both AttachPodRoutes() AttachPodMetricsRoutes() with the same mux.