guidonguido commented 2 years ago

Description

This PR introduces a Custom Metrics Server that provides CustomMetrics to clients via gRPC. Among others, it is required to evaluate QoE for container-based instances.

The Custom Metrics Server requires the CRI-API runtime v1alpha2 to be running on the cluster.

kingmakerbot commented 2 years ago

Hi @guidonguido. Thanks for your PR.

I am @kingmakerbot. You can interact with me issuing a slash command in the first line of a comment. Currently, I understand the following commands:

/rebase: Rebase this PR onto the master branch
/merge: Merge this PR into the master branch
/hold: Adds hold label to prevent merging with /merge
/unhold: Removes the hold label to allow merging with /merge
/deploy-staging: Deploy a staging environment to test this PR (the build-all flag enables user environments building)
/undeploy-staging: Manually undeploy the staging environment

Make sure this PR appears in the CrownLabs changelog, adding one of the following labels:

kind/breaking: :boom: Breaking Change
kind/feature: :rocket: New Feature
kind/bug: :bug: Bug Fix
kind/cleanup: :broom: Code Refactoring
kind/docs: :memo: Documentation

guidonguido commented 2 years ago

PTAL ⚡

giorio94 commented 2 years ago

Looking at the code, it seems to me it is ready for a try in the staging environment. Still, you should first add an entry in the build-matrix.json file to ensure the instmetrics container gets built by the CI pipeline.

giorio94 commented 2 years ago

Seems good, I left a couple of style related comments and stuff. Still I'm not really sure about the several times in which you make new contexts just to avoid passing the logger as a new parameter... In general it's a practice that I think it's more useful when you have to traverse several layers of functions sharing the same context, however most of the times I think just passing the logger as a separate parameter could be still more performant and less impacting for memory...

Relying also here, just for the sake of completeness. I am personally not concerned about the possible overhead of defining a new context, which should be practically close to the definition of a variable. The overhead of this code comes from the calls to the CRI API, and we should spend time optimizing those (if that is really a concern), all the rest is negligible. Do not try to optimize the nits, leaving behind what really matters.

Instead, I am more concerned about how the logs would actually look with all those different names, but that is easier to see when deployed in CrownLabs.