fix(metrics): remove resources that aren't updated #637
Conversation
openshift-ci-robot
added
approved
openshift-ci-robot
added
size/XXL
|
/retest |
1 similar comment
|
/retest |
|
/skip gitlab-ci/pipeline |
| go http.ListenAndServe(":8080", nil) | ||
|
|
||
| http.Handle("/metrics", promhttp.Handler()) | ||
| go http.ListenAndServe(":8081", nil) |
There was a problem hiding this comment.
Also serving a nil handler here
There was a problem hiding this comment.
Yes, nil is intentional, it should use the "DefaultServeMux".
There was a problem hiding this comment.
Gotcha - didn't know about DefaultServeMux - I think go didn't have a muxer last time I used http? 😂
| @@ -87,6 +93,10 @@ func main() { | |||
| log.Panicf("error configuring operator: %s", err.Error()) | |||
| } | |||
|
|
|||
| http.Handle("/metrics", promhttp.Handler()) | |||
| go http.ListenAndServe(":8081", nil) | |||
There was a problem hiding this comment.
This is serving a nil handler - did you mean to serve metrics on this port?
There was a problem hiding this comment.
This port change was intentional, though perhaps it's not the best? OLM serves healthz from port 8080, so I wanted the metrics ports to be consistent.
| // TODO: both of the following require vendor updates (add k8s.io/apiserver and update prometheus) | ||
| //healthz.InstallHandler(mux) //(less code) | ||
| //mux.Handle("/metrics", promhttp.Handler()) //other form is deprecated | ||
| http.Handle("/metrics", prometheus.Handler()) | ||
| go http.ListenAndServe(":8080", nil) |
There was a problem hiding this comment.
Similar issue here, though this has clearly been here a while
There was a problem hiding this comment.
See other comments, I'm nearly certain I've confirmed that healthz was being served correctly.
|
/lgtm |
|
/skip gitlab-ci/pipeline |
openshift-ci-robot
added
the
needs-rebase
|
@jpeeler I'm optimistic about this merging if you rebase? |
Make the resources that are updated by the catalog operator be served from there too (and remove them from olm). Update the prometheus http handler to use the non-deprecated version, and do it on port 8081. Also, actually serve the health check in OLM.
openshift-ci-robot
removed
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, jpeeler 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 |
|
/skip gitlab-ci/pipeline |
|
/refresh |
|
/skip |
fix(metrics): remove resources that aren't updated
These resources are updated in the catalog operator, so a new endpoint
needs to be created to scrape them from there instead. This commit
merely removes them from being served from the olm operator.
(So this is really just a partial fix.)Updated to actually serve the new metrics properly.