Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add ServiceMonitor for Prometheus operator #58

Merged
merged 6 commits into from
Feb 22, 2023

Conversation

nikolaik
Copy link
Contributor

@nikolaik nikolaik commented Feb 9, 2023

Description of the change

Backstage instances with the Prometheus /metrics endpoint configured can set metrics.serviceMonitor.enabled to allow prometheus to scrape it's metrics

Disabled by default.

Example diff:

backstage, backstage, ServiceMonitor (monitoring.coreos.com) has been added:
+ # Source: backstage/templates/servicemonitor.yaml
+ apiVersion: monitoring.coreos.com/v1
+ kind: ServiceMonitor
+ metadata:
+   name: backstage
+   namespace: "backstage"
+   labels:
+     app.kubernetes.io/name: backstage
+     helm.sh/chart: backstage-0.16.0
+     app.kubernetes.io/instance: backstage
+     app.kubernetes.io/managed-by: Helm
+     app.kubernetes.io/component: backstage
+ spec:
+   namespaceSelector:
+     matchNames:
+       - "backstage"
+   selector:
+     matchLabels:
+       app.kubernetes.io/name: backstage
+       helm.sh/chart: backstage-0.16.0
+       app.kubernetes.io/instance: backstage
+       app.kubernetes.io/managed-by: Helm
+       app.kubernetes.io/component: backstage
+   endpoints:
+   - port: http-backend
+     path: /metrics

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

@nikolaik nikolaik requested a review from a team as a code owner February 9, 2023 13:31
Backstage instances with the Prometheus /metrics endpoint configured [1]
can set `serviceMonitor.enabled` to allow prometheus to scrape it's metrics

Disabled by default.

[1] https://github.com/backstage/backstage/blob/master/contrib/docs/tutorials/prometheus-metrics.md

Signed-off-by: Nikolai R Kristiansen <nikolai.kristiansen@remarkable.no>
@nikolaik nikolaik force-pushed the feat/servicemonitor branch from 750afb8 to fe1cba2 Compare February 9, 2023 13:31
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @nikolaik! 🙂 While I like your PR in general, I have conflicting feelings, namely because:

  1. ServiceMonitor is a CR, it's not part of vanilla kubernetes and you need to have Prometheus operator installed. This introduces undisclosed dependency.
  2. Default, vanilla Backstage instance has no metrics endpoint. This introduces configuration requirement on the Backstage image side.

While I agree with the purpose, I'm not entirely sure this should be part of a canonical chart but rather an opinionated extension of it.

What do others think? cc @backstage/helm-charts

@ChrisJBurns
Copy link
Contributor

I'm not that against this feature if I'm being honest. Mainly because by default it won't be enabled which will allow folks to be able to turn it on if they ever want it. The second point touches on a more crucial point for me though which is that for this functionality to work, the underlying backstage image would have to have the metrics code.

On the whole, I do agree that we do need to be careful on what we allow as non vanilla Kubernetes features, although if I'm being honest, we can probably make those judgements on an Issue/PR basis. This for example is something that I believe would be good to offer in the Chart because monitoring is quite a big deal in Kubernetes - the only caveat to this is that it assumes that the Prometheus operator is installed. Although by default it isn't enabled so for me its not the end of the world.

Interested to hear others thoughts on this

@vinzscam
Copy link
Member

Looks good to me. Nice addition @nikolaik! 🙏

charts/backstage/values.yaml Outdated Show resolved Hide resolved
charts/backstage/values.yaml Outdated Show resolved Hide resolved
charts/backstage/values.yaml Outdated Show resolved Hide resolved
charts/backstage/values.yaml Outdated Show resolved Hide resolved
charts/backstage/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
charts/backstage/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
charts/backstage/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
@nikolaik
Copy link
Contributor Author

Thanks for the review @tumido <3 I'll update the PR with your feedback shortly.

And thank you for bringing up the pros and cons of including ServiceMonitor in the chart. I went with disabled by default because using the option requires non default functionality in backstage. I'll point users to the metrics endpoint tutorial and some other relevant docs where it makes sense in the values.yaml file.

One point that is not mentioned is that ServiceMonitor ties into the CNCF ecosystem of packages, which both backstage and prometheus is part of, although I see that the prometheus-operator is not part of the official prometheus project.

- Move to `metrics.serviceMonitor
- Document prometheus-operator and /metrics endpoint requirements
- Remove namespaceSelector config option
- Add commonLabels and commonAnnotations

Signed-off-by: Nikolai R Kristiansen <nikolai.kristiansen@remarkable.no>
@nikolaik nikolaik force-pushed the feat/servicemonitor branch from f579f9b to ab5b2df Compare February 14, 2023 13:28
Signed-off-by: Nikolai R Kristiansen <nikolai.kristiansen@remarkable.no>
@nikolaik nikolaik force-pushed the feat/servicemonitor branch from 50f485b to 3b9614e Compare February 14, 2023 13:42
@nikolaik nikolaik force-pushed the feat/servicemonitor branch from fc87190 to 783140f Compare February 15, 2023 08:04
Co-authored-by: Vincenzo Scamporlino <me@vinzscam.dev>
Signed-off-by: Nikolai R Kristiansen <nikolai.kristiansen@remarkable.no>
@nikolaik nikolaik force-pushed the feat/servicemonitor branch from 783140f to d5bc138 Compare February 15, 2023 08:22
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing I've noticed... We should maintain consistency with all other resources in label/annotation templating and rendering.

charts/backstage/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
charts/backstage/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
@nikolaik
Copy link
Contributor Author

One last thing I've noticed... We should maintain consistency with all other resources in label/annotation templating and rendering.

Sorry for being unresponsive, fixing the last bits now...

@nikolaik nikolaik requested a review from tumido February 22, 2023 15:30
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries at all! 🙂

/lgtm
/approve

Looks great now 👍

@tumido
Copy link
Member

tumido commented Feb 22, 2023

@nikolaik CI is complaining, can you please fix this?
image

https://probot.github.io/apps/dco/

Signed-off-by: Nikolai R Kristiansen <nikolai.kristiansen@remarkable.no>
@nikolaik nikolaik force-pushed the feat/servicemonitor branch from 5a1b07e to 2e31c1a Compare February 22, 2023 20:53
@nikolaik
Copy link
Contributor Author

@nikolaik CI is complaining, can you please fix this? image

https://probot.github.io/apps/dco/

Sorry about that, fixed ✅

@tumido tumido merged commit 37b9b30 into backstage:main Feb 22, 2023
@nikolaik nikolaik deleted the feat/servicemonitor branch February 23, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants