feature: optimise the logic of using prometheus #2429

fengzixu · 2018-11-05T17:31:36Z

Signed-off-by: fengzixu [email protected]

Ⅰ. Describe what this PR did

use the custom registry to replace the default.
upgrade the package called client-go of prometheus
add the new dependent package called promhttp
enable metrics about gRPC server

Related to: #2395

Ⅱ. Does this pull request fix one issue?

#2395 #2414

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

First, I complied a new binary pouchd, and I run it with parameters:

 /home/xr/workspace/go-project/src/github.com/alibaba/pouch/bin/pouchd -l tcp://0.0.0.0:4243 -l unix:///var/run/pouchd.sock --enable-cri=true --cri-version=v1alpha2

Second, I visted successfully the url: http://<hostIP>:4243/metrics so that I can capture metrics produced by pouchd.

And then, I check some points about metrics:

Is there all metrics was registered successfully?

I saved the result about metrics: https://gist.github.com/fengzixu/ab018bd1d1fa8ddc56a6fe9150d36feb. I picked up some samples and compared with original metrics in code. There is no any difference.

Are the active metrics show commonly after I process some operations ?

I use the command pouch pull nginx to pull a image, and I re-captured the metrics from pouch. The figure of metrics about image has a little increase.

Whether the metrics about gRPC server show commonly?

I started up a k8s cluster together with pouch. And I created some pods. After that I found that the metrics about gRPC between pouchd and kubelet has some figures. It was beneficial for me when I debug this function.

Ⅴ. Special notes for reviews

codecov · 2018-11-05T17:31:38Z

Codecov Report

Merging #2429 into master will decrease coverage by 0.23%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2429      +/-   ##
==========================================
- Coverage    68.8%   68.57%   -0.24%     
==========================================
  Files         272      272              
  Lines       18159    18175      +16     
==========================================
- Hits        12495    12464      -31     
- Misses       4248     4279      +31     
- Partials     1416     1432      +16

Flag	Coverage Δ
#criv1alpha1test	`31.54% <100%> (+0.26%)`	⬆️
#criv1alpha2test	`35.61% <100%> (-0.11%)`	⬇️
#integrationtest	`39.97% <100%> (-0.13%)`	⬇️
#nodee2etest	`32.82% <100%> (-0.05%)`	⬇️
#unittest	`26.52% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
apis/server/router.go	`85.71% <100%> (ø)`	⬆️
apis/metrics/metrics.go	`100% <100%> (ø)`	⬆️
pkg/utils/metrics/metrics.go	`100% <100%> (ø)`	⬆️
cri/metrics/metrics.go	`100% <100%> (ø)`	⬆️
cri/ocicni/cni_manager.go	`59.18% <0%> (-12.25%)`	⬇️
ctrd/watch.go	`75.75% <0%> (-7.58%)`	⬇️
cri/stream/portforward/httpstream.go	`69.16% <0%> (-6.67%)`	⬇️
daemon/mgr/system.go	`67.93% <0%> (-5.35%)`	⬇️
daemon/mgr/events.go	`96.29% <0%> (-3.71%)`	⬇️
cri/stream/runtime.go	`67.85% <0%> (-2.39%)`	⬇️
... and 11 more

fengzixu · 2018-11-05T17:35:14Z

@starnop After I developed this PR and thought deeply about metrics of gRPC server. I considered that we should save the gRPC metrics. That was beneficial for developers when they want to develop some functions, which perhaps were associated with communication between kubelet and pouchd.

fengzixu · 2018-11-05T17:36:27Z

Request review.

fengzixu · 2018-11-05T17:38:19Z

By the way, the main reason I upgrade the package called github.com/prometheus/client_golang/prometheus is that I want to use package called promehttp.

vendor/vendor.json

fuweid · 2018-11-06T01:40:44Z

By the way, the main reason I upgrade the package called github.com/prometheus/client_golang/prometheus is that I want to use package called promehttp.

Sorry, I din't get the point about this PR. Could you explain more detail about what is good from using customize registry? Thanks

fengzixu · 2018-11-06T01:45:46Z

By the way, the main reason I upgrade the package called github.com/prometheus/client_golang/prometheus is that I want to use package called promehttp.

Sorry, I din't get the point about this PR. Could you explain more detail about what is good from using customize registry? Thanks

I describe some details about this PR in #2395. @fuweid

fuweid · 2018-11-06T01:51:49Z

According to those comments about DefaultRegisterer, we can know that there is a big risk about using global variant in our code especially in the dependent package. If someone modifies this variant accidentally through some methods written in prometheus. That is a disaster. That is a reason, we should avoid using global variants as much as possible.

Good to know. Thanks

fuweid

LGTM

starnop · 2018-11-06T02:13:08Z

vendor/vendor.json

@@ -1930,4 +1936,4 @@
 		}
 	],
 	"rootPath": "github.com/alibaba/pouch"
-}
+}


There should not be a change, please handle it, THX.

starnop · 2018-11-06T02:16:41Z

Except for the little problem above, LGTM. Thanks for your contribution. :)

HusterWan · 2018-11-06T02:57:03Z

apis/server/router.go

@@ -88,8 +89,8 @@ func initRoute(s *Server) http.Handler {
 	s.addRoute(r, http.MethodPost, "/networks/{id:.*}/disconnect", s.disconnectNetwork)

 	// metrics
-	r.Path(versionMatcher + "/metrics").Methods(http.MethodGet).Handler(prometheus.Handler())
-	r.Path("/metrics").Methods(http.MethodGet).Handler(prometheus.Handler())
+	r.Path(versionMatcher + "/metrics").Methods(http.MethodGet).Handler(promhttp.HandlerFor(util_metrics.GetCustomPrometheusRegistry(), promhttp.HandlerOpts{}))


How to make the function length short?

Ok. Let me fix it.

I was wondering if you could set the prometheus handler using s.addRoute?
It would be the similar style as the other handler.

1. use the custom registry to replace the default. 2. upgrade the package called client-go of prometheus 3. add the new dependent package called promhttp Related to: AliyunContainerService#2395 Signed-off-by: fengzixu <[email protected]>

fengzixu · 2018-11-06T05:06:55Z

@HusterWan @starnop Request review again. I have fixed some problems above.

fuweid · 2018-11-06T05:18:15Z

pkg/utils/metrics/metrics.go

@@ -63,3 +80,21 @@ func NewLabelTimer(subsystem, name, help string, labels ...string) *prometheus.H
 			ConstLabels: nil,
 		}, labels)
 }
+
+// GetCustomPrometheusRegistry create a custom resigtry of Prometheus.
+func GetCustomPrometheusRegistry() *prometheus.Registry {


I think we should use GetPrometheusRegistry here. The Custom is for the prometheus package, not for pouch. Right?

hmmm, that is right. As a developer of pouch, he(she) should not care about custom or default.

HusterWan · 2018-11-06T06:55:29Z

lgtm

allencloud · 2018-11-06T07:00:06Z

Thanks for your contribution. @fengzixu

fuweid · 2018-11-06T07:01:52Z

@fengzixu could you follow up to change the name? thanks

fengzixu · 2018-11-06T07:23:17Z

@fengzixu could you follow up to change the name? thanks

Sure. I will submit a pr to modify the name. @fuweid

feature: modify the function name about prometheus registry 1. change function name `GetCustomPrometheusRegistry` to `GetPrometheusRegistry` Related to: AliyunContainerService#2429

feature: modify the function name about prometheus registry 1. change function name `GetCustomPrometheusRegistry` to `GetPrometheusRegistry` Related to: #2429

pouchrobot added areas/log areas/monitoring kind/feature size/XXL labels Nov 5, 2018

allencloud reviewed Nov 6, 2018

View reviewed changes

vendor/vendor.json Show resolved Hide resolved

allencloud assigned starnop Nov 6, 2018

fengzixu force-pushed the revise-prometheus branch from b7e0b70 to ced1703 Compare November 6, 2018 01:27

fuweid reviewed Nov 6, 2018

View reviewed changes

starnop reviewed Nov 6, 2018

View reviewed changes

HusterWan reviewed Nov 6, 2018

View reviewed changes

feature: optimise the logic of using prometheus

5f04755

1. use the custom registry to replace the default. 2. upgrade the package called client-go of prometheus 3. add the new dependent package called promhttp Related to: AliyunContainerService#2395 Signed-off-by: fengzixu <[email protected]>

fengzixu force-pushed the revise-prometheus branch from ced1703 to 5f04755 Compare November 6, 2018 05:05

fuweid reviewed Nov 6, 2018

View reviewed changes

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 6, 2018

allencloud merged commit bc76c24 into AliyunContainerService:master Nov 6, 2018

fengzixu mentioned this pull request Nov 6, 2018

feature: change the function name about prometheus registry #2436

Merged

fuweid pushed a commit that referenced this pull request Nov 6, 2018

Signed-off-by: fengzixu <[email protected]>

26b6d5f

feature: modify the function name about prometheus registry 1. change function name `GetCustomPrometheusRegistry` to `GetPrometheusRegistry` Related to: #2429

pouchrobot mentioned this pull request Nov 9, 2018

WeeklyReport of PouchContainer from 2018-11-02 to 2018-11-09 #2457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: optimise the logic of using prometheus #2429

feature: optimise the logic of using prometheus #2429

fengzixu commented Nov 5, 2018 •

edited

Loading

codecov bot commented Nov 5, 2018 •

edited

Loading

fengzixu commented Nov 5, 2018

fengzixu commented Nov 5, 2018

fengzixu commented Nov 5, 2018

fuweid commented Nov 6, 2018

fengzixu commented Nov 6, 2018

fuweid commented Nov 6, 2018

fuweid left a comment

starnop Nov 6, 2018

starnop commented Nov 6, 2018

HusterWan Nov 6, 2018

fengzixu Nov 6, 2018

CodeJuan Nov 6, 2018

fengzixu commented Nov 6, 2018

fuweid Nov 6, 2018

fengzixu Nov 6, 2018

HusterWan commented Nov 6, 2018

allencloud commented Nov 6, 2018

fuweid commented Nov 6, 2018

fengzixu commented Nov 6, 2018

feature: optimise the logic of using prometheus #2429

feature: optimise the logic of using prometheus #2429

Conversation

fengzixu commented Nov 5, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Is there all metrics was registered successfully?

Are the active metrics show commonly after I process some operations ?

Whether the metrics about gRPC server show commonly?

Ⅴ. Special notes for reviews

codecov bot commented Nov 5, 2018 • edited Loading

Codecov Report

fengzixu commented Nov 5, 2018

fengzixu commented Nov 5, 2018

fengzixu commented Nov 5, 2018

fuweid commented Nov 6, 2018

fengzixu commented Nov 6, 2018

fuweid commented Nov 6, 2018

fuweid left a comment

Choose a reason for hiding this comment

starnop Nov 6, 2018

Choose a reason for hiding this comment

starnop commented Nov 6, 2018

HusterWan Nov 6, 2018

Choose a reason for hiding this comment

fengzixu Nov 6, 2018

Choose a reason for hiding this comment

CodeJuan Nov 6, 2018

Choose a reason for hiding this comment

fengzixu commented Nov 6, 2018

fuweid Nov 6, 2018

Choose a reason for hiding this comment

fengzixu Nov 6, 2018

Choose a reason for hiding this comment

HusterWan commented Nov 6, 2018

allencloud commented Nov 6, 2018

fuweid commented Nov 6, 2018

fengzixu commented Nov 6, 2018

fengzixu commented Nov 5, 2018 •

edited

Loading

codecov bot commented Nov 5, 2018 •

edited

Loading