Skip to content
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

[question] Whether the logics about metrics in cri dir needs to revise? #2395

Closed
fengzixu opened this issue Oct 31, 2018 · 4 comments
Closed
Assignees
Labels
areas/log kind/question all questions or confusion about this project

Comments

@fengzixu
Copy link
Contributor

fengzixu commented Oct 31, 2018

Question

When I read codes about metrics of gRPC in the pouch, I found that there are some not standard approaches in using the object called Registerer in Prometheus.

Actually, in https://github.com/alibaba/pouch/blob/master/cri/metrics/metrics.go#L78, the method called MustRegister registers metrics into the register variant in Prometheus. But, that is a default and global register. We can see this behaviour in https://github.com/prometheus/client_golang/blob/0a8115f42e037a6e327f9a269a26ff6603fb8472/prometheus/registry.go#L51 .

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.

I contributed some examples to go-grpc-prometheus. In that PR, the member of Prometheus give me some advice about using register:

  1. Using NewRegistry method to create a register variant which owned by yourself.
  2. Registering all metrics which you cared for, including some default metrics or custom metrics.

Ref: grpc-ecosystem/go-grpc-prometheus#35 (comment)

So, if someone thinks that we should optimize the logics about using prometheus. I'm very glad to handle it. I have some experiences in there.

@pouchrobot pouchrobot added areas/log kind/question all questions or confusion about this project labels Oct 31, 2018
@fengzixu
Copy link
Contributor Author

We should supply a standard pattern of using Prometheus in pouch.

@allencloud
Copy link
Collaborator

cc @starnop @CodeJuan

@starnop
Copy link
Contributor

starnop commented Nov 1, 2018

@fengzixu Thank you very much for your advice. In fact, so do that with the apis/metrics .
We are very glad that if you are willing to help with this. ^_^

@fengzixu
Copy link
Contributor Author

fengzixu commented Nov 1, 2018

Sure. I'm very glad to handle it.

fengzixu added a commit to fengzixu/pouch that referenced this issue Nov 5, 2018
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 added a commit to fengzixu/pouch that referenced this issue Nov 6, 2018
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 added a commit to fengzixu/pouch that referenced this issue Nov 6, 2018
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]>
allencloud pushed a commit that referenced this issue Nov 6, 2018
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:  #2395
Signed-off-by:  fengzixu <[email protected]>
@fengzixu fengzixu closed this as completed Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/log kind/question all questions or confusion about this project
Projects
None yet
Development

No branches or pull requests

4 participants