charts/*: drop wireService label, use app= instead, add servicemonitor support#2413
charts/*: drop wireService label, use app= instead, add servicemonitor support#2413
Conversation
jschaul
left a comment
There was a problem hiding this comment.
looks fine to me; if CI passes.
I'm not sure if the wire-server-metrics chart truly used anywhere by anyone (not if it's installed, but if anyone is looking at any of those metrics)
|
Then I'd probably even propose going forward to entirely removing the |
|
The wire-server-metrics chart is used by the federation environments. So, if you do change this flow, please don't forget to update the federation environments. Hopefully, this change doesn't blow up there 😄 |
Are we fine with just stopping to instantiate the |
We added metrics there because it was hard for us to figure out what was wrong when things didn't work. So, I would say maybe this is still needed, but best ask people working on federation these days. /cc @smatting @pcapriotti @mdimjasevic @stephen-smith |
I don't think we have used these metrics for any debugging yet. @supersven have you made use of them while debugging calling? If not I think it's fine to remove it, wdyt?
Would this also be eventually available for the federation environments? |
|
The wire-server-metrics chart works fine as it is right now, and has no dependencies on other things, it even has up-to-date docs in https://docs.wire.com/how-to/install/monitoring.html So I don't see any reason to break a so-far working chart without providing alternatives. If things are temporarily broken for less than 1 week and you intend to restore the behaviour of wire-server-metrics chart working as before; then I have no issue with this PR. If the intention is to make a whole new monitoring capability at some point in the far future and in the meantime things will be broken on all environments that use wire-server-metrics; then I'm not a fan. I'm not familiar enough with service probes and your overall intention to understand what you'd like to do; please explain. |
Nope. Debugging calling is currently more looking at logs and reasoning about Helm charts. |
|
I guess we can just keep this PR open until it also adds ServiceProbes, which provides an alternative. |
d73bbd7 to
e582cb0
Compare
|
I went through all services in
For those that did, I created the necessary helm file to create a As the CRDs don't come shipped with Kubernetes out of the box, but they're usually installed while installing a monitoring operator, it's opt-in and disabled by default. |
This aligns labels a bit more with how they look like in other deployments. In some cases, we were already setting the `app` label, too. There's one possible regression: The wire-server-metrics helm chart configured kube-prometheus-stack to automatically scrape everything with a wireService label at port http, path /i/metrics. This will be fixed in a followup, by adding ServiceProbe resources to each workload that exposes metrics.
44e3299 to
bacf9c9
Compare
|
Hmm, it seems it's not possible to modify the |
|
For posterity: release notes to document the upgrade were added in #2472. |
This aligns labels a bit more with how they look like in other
deployments. In some cases, we were already setting the
applabel,too.
There's one possible regression:
The wire-server-metrics helm chart previously configured kube-prometheus-stack to automatically scrape everything with a wireService label at port http,
path /i/metrics.
This custom configuration has been removed, and instead each chart provides the option to create
ServiceMonitorresources, which add wire services to metric scraping that way.Checklist
make git-add-cassandra-schemato update the cassandra schema documentation.changelog.d.