move proxy metrics to a separate server #352
Conversation
ranakan19
requested review from
sbryzak,
xcoulon,
alexeykazakov,
MatousJobanek,
mfrancisc and
rajivnathan
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
There was a problem hiding this comment.
We also need to expose the 8082 port in our container:
registration-service/build/Dockerfile
Line 19 in b4cc745
| @@ -98,6 +98,7 @@ objects: | |||
| ports: | |||
| - containerPort: 8080 | |||
| - containerPort: 8081 | |||
| - containerPort: 8082 | |||
There was a problem hiding this comment.
This file is used only in dev environments. The one which is used in production (and e2e tests) are defined here: https://github.com/codeready-toolchain/host-operator/blob/master/deploy/registration-service/registration-service.yaml
Please create a PR to update that file too.
But what I'm wonder is why the e2e-test still pass? Do we have any e2e tests for this metric service? Without updating https://github.com/codeready-toolchain/host-operator/blob/master/deploy/registration-service/registration-service.yaml the e2e-tests for this PR should fail...
There was a problem hiding this comment.
could we also add names to the container ports (see for example https://kubernetes.io/docs/concepts/services-networking/service/#field-spec-ports) to make them more explicit, and reuse these names in the services as well?
There was a problem hiding this comment.
This file is used only in dev environments.
It's actually not used even in dev environment. This file is a legacy thing from the time when we were able to deploy reg-service on its own - we don't do that anymore (or at least I'm not aware of anyone who would be doing that)
There was a problem hiding this comment.
but anyway, Alexey is right that you need to create PR in host-repo with this change. Let's do it in two steps:
- create PR in host-operator repo changing only the values in the template (I don't think that we verify the actual content of the deployment in e2e tests)
- when the host-operator PR is merged, create a PR in e2e tests that will be paired with this reg-service one that will verify that the
Serviceis created.
There was a problem hiding this comment.
Sorry for the late reply,
@MatousJobanek @alexeykazakov Thanks for your comments and catching the missing host-repo changes. I did have the changes in host-repo, that's how i was able to test it locally indeed, but had missed creating the PR on host-repo. I've updated the host-operator and e2e PR in the description.
| @@ -98,6 +98,7 @@ objects: | |||
| ports: | |||
| - containerPort: 8080 | |||
| - containerPort: 8081 | |||
| - containerPort: 8082 | |||
There was a problem hiding this comment.
but anyway, Alexey is right that you need to create PR in host-repo with this change. Let's do it in two steps:
- create PR in host-operator repo changing only the values in the template (I don't think that we verify the actual content of the deployment in e2e tests)
- when the host-operator PR is merged, create a PR in e2e tests that will be paired with this reg-service one that will verify that the
Serviceis created.
pkg/metrics/metrics.go
Outdated
|
|
||
| func StartMetricsServer() *http.Server { | ||
| // start server | ||
| router := echo.New() |
There was a problem hiding this comment.
router looks like a copy-paste zombie from the proxy code :-)
pkg/metrics/metrics_test.go
Outdated
| // check the status code is what we expect, and the content-type | ||
| require.Equal(t, http.StatusOK, rec.Code) | ||
| require.Equal(t, "text/plain; version=0.0.4; charset=utf-8", rec.Header().Get("Content-Type")) |
There was a problem hiding this comment.
does it make sense to verify that the body also contains the actual registered metric? you know, just check that the name of the metric is there, no need to verify the actual values...
…ice into allowMetrics
|
/retest |
|
Kudos, SonarCloud Quality Gate passed!
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, ranakan19 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 |
move proxy metrics to a separate server
host-operator PR - codeready-toolchain/host-operator#884
paired e2e PR - codeready-toolchain/toolchain-e2e#816