-
Notifications
You must be signed in to change notification settings - Fork 947
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 prometheus vendor and pull images api metric and go-related metric like gc time #91
Conversation
Thanks for your contribution. 🍻 @WIZARD-CXY |
With basic prometheus package we can also have go-related metrics |
Could you please guide us how to utilize the feature of Prometheus integration? |
// record the time spent during image pull procedure. | ||
defer func(start time.Time) { | ||
metrics.ImagePullSummary.WithLabelValues(image + ":" + tag).Observe(metrics.SinceInMicroseconds(start)) | ||
}(time.Now()) |
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 think the code should be moved into "apis/server/router.go: filter()", so that We don't need to insert some code into many places.
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 pr is focusing on a simple pull image duration metric. No need to move it to filter now. Besides I see
r.Path("/_ping").Methods(http.MethodGet).Handler(s.filter(s.ping))
r.Path("/info").Methods(http.MethodGet).Handler(s.filter(s.info))
r.Path("/version").Methods(http.MethodGet).Handler(s.filter(s.version))
we don't need to record ping info and version api latency.
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 think it is not the blocker for this pr. feel free to add it in the future pr @skoo87
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 still insist that we should add a doc for Prometheus.
Once merged, one will never add the doc any more, believe me. @WIZARD-CXY
WDYT?
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.
will add in another pr @allencloud
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.
OK, thanks a lot.
And could you add this before weekends? @WIZARD-CXY
apis/server/image_bridge.go
Outdated
|
||
"github.com/alibaba/pouch/apis/metrics" | ||
"github.com/sirupsen/logrus" |
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.
Add a blank line before "logrus"
Now with this pr, we will have so many useful metrics
|
indicates pouchd take 37s to pull docker.io/library/ubuntu:latest image |
@allencloud I will add a new documentation to show how to add new metrics and some conventions. I think we can delegate the api request lantency metrics to the sel guys. |
…-related metrics. Signed-off-by: 宇慕 <xingyu.chenxingyu@alibaba-inc.com>
First, for end-users, how to use prometheus in pouch, this is the most important. Second, for develop-user, how to add new metrics to pouch is the second most important thing. @WIZARD-CXY |
@allencloud good point |
I would like to hear some thoughts from @skoo87. |
I would like to merge this first. And we can iterate fast. |
LGTM |
1.Describe what this PR did
add prometheus package into vendor and a pull image latency monitoring.
This PR is related to issue #16
2.Does this pull request fix one issue?
3.Describe how you did it
add code for monitoring in the apiserver part.
4.Describe how to verify it
use pouch to pull a image and get metrics from /metrics http endpoint to verify.
5.Special notes for reviews
NONE