-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ add metrics for webhooks #234
Conversation
21d3427
to
c8344a0
Compare
pkg/webhook/admission/webhook.go
Outdated
@@ -121,6 +123,8 @@ func (w *Webhook) Handle(ctx context.Context, req atypes.Request) atypes.Respons | |||
case types.WebhookTypeValidating: | |||
resp = w.handleValidating(ctx, req) | |||
default: | |||
metrics.FailedRequests.WithLabelValues(w.Name).Inc() |
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.
I have a general question. Maybe I missed that part. It looks that the failure branches will have metrics. How about the successful ones?
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 is a WIP PR.
I created this PR to see if Travis can reproduce the same strange error I have locally.
I will push more change for the successful ones.
1b33798
to
9de9139
Compare
pkg/webhook/admission/http.go
Outdated
encoder := json.NewEncoder(w) | ||
responseAdmissionReview := v1beta1.AdmissionReview{ | ||
Response: response.Response, | ||
} | ||
err := encoder.Encode(responseAdmissionReview) | ||
if err != nil { | ||
log.Error(err, "unable to encode the response") | ||
writeResponse(w, ErrorResponse(http.StatusInternalServerError, err)) | ||
metrics.FailedRequests.WithLabelValues(wh.Name).Inc() |
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.
why are we incrementing both here and right before we call this method?
// the webhook server has successfully processed. | ||
SucceededRequests = prometheus.NewCounterVec( | ||
prometheus.CounterOpts{ | ||
Name: "controller_runtime_webhook_succeeded_requests", |
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 be a label on requests (like succeeded
as the label, and then true
and false
as the values)
// the webhook server has received. | ||
TotalRequests = prometheus.NewCounterVec( | ||
prometheus.CounterOpts{ | ||
Name: "controller_runtime_webhook_total_requests", |
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.
requests_total
. By convention, all counters end in _total
.
// the webhook server has failed to process with an server internal error. | ||
InternalErrorRequests = prometheus.NewCounterVec( | ||
prometheus.CounterOpts{ | ||
Name: "controller_runtime_webhook_internal_error_requests", |
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.
again, this should be a label on requests_total
.
Please make sure you review https://github.com/kubernetes/community/blob/master/contributors/devel/instrumentation.md and https://prometheus.io/docs/practices/naming/ when contributing metrics. |
fad4014
to
ff81845
Compare
PTAL |
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.
Looks good to me. Minor suggest to rename.
|
||
// Duration is a prometheus metric which is a histogram of the latency | ||
// of processing an admission request. | ||
Duration = prometheus.NewHistogramVec( |
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.
We should rename it to RequestLatency ?
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.
Renamed
ff81845
to
84b388b
Compare
PTAL |
@DirectXMan12 PTAL |
Maybe this is a follow-on PR, but it would be nice to document in the go doc what metrics are registered when using this package? |
/lgtm /approve |
Bot seems to be dead :/ |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: mengqiy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add 2 metrics for webhook: