samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes
Apache License 2.0
110 stars 24 forks source link

Prometheus metrics exporter for samba-operator #160

Closed synarete closed 2 years ago

synarete commented 2 years ago

Provide Prometheus metrics-exporter container and deploy is along-side (i.e., within same pod as) the samba-server container. Use 'smbstatus' to query smbd and export it over HTTP port 8080.

phlogistonjohn commented 2 years ago

I've done a quick skim and the general approach looks great. I'm so happy to see this.

One thing I'd like to discuss early on is where the code responsible for the metrics container should live. I have no issues "incubating" the prototype in the operator repo. I did that myself with the svcwatch utility. However, much like svcwatch I feel that the code responsible for parsing the smbstatus output and producing the metrics http endpoint should probably live outside the operator repo.

I recognize there are some downsides to that, but here's a short list of reasons why I think it is preferable to create a new repo for it:

synarete commented 2 years ago

I've done a quick skim and the general approach looks great. I'm so happy to see this.

One thing I'd like to discuss early on is where the code responsible for the metrics container should live. I have no issues "incubating" the prototype in the operator repo. I did that myself with the svcwatch utility. However, much like svcwatch I feel that the code responsible for parsing the smbstatus output and producing the metrics http endpoint should probably live outside the operator repo.

I recognize there are some downsides to that, but here's a short list of reasons why I think it is preferable to create a new repo for it:

  • Having a Prometheus endpoint for samba is useful outside the operator or even containerized samba use cases. These potential users should not have to "deal with" the rest of the operator infrastructure to make use of that.
  • Having a mixed repository leads to mixed dependencies (in go.mod). Without looking deeper into the code its not clear what parts of the repo use what dependency. While this pr adds little that may expand in the future. Tooling that largely depends on go mod to detect dependencies and reports new versions or vulnerabilities may cause it to seem like the operator has a dependency with an issue but its only the metrics endpoint (or vice versa!)
  • A more streamlined code base. Someone, especially someone new, wouldn't need to become familiar with the more complex makefile or directory structure of the operator if small changes were needed for the metric exporter.
  • It's more discoverable. It's more obvious that the sink org has a metrics exporter if there's a URL with a readme, etc. Rather than having to look into the operator and seeing an exporter mixed in with all the other operator stuff.
  • In the long term I think its probably beneficial to have different releases/versions for these code bases. Admittedly, its annoying now in the short term when we haven't officially released anything.

Agreed: it would be better to separate the smbmetrics code into its own repository and let samba-operator consume its image. Started this work[1], which will turn this PR into a draft-only.

[1] https://github.com/samba-in-kubernetes/smbmetrics

synarete commented 2 years ago

Metrics is now supported via smbmetrics: https://github.com/samba-in-kubernetes/smbmetrics where samba-operator execute it as a sidecar.