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 extra labels on serviceMonitor + allow toleration configuration on helm chart #303

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

mmadoo
Copy link
Contributor

@mmadoo mmadoo commented Apr 25, 2023

Improve helm chart in order to

  • allow to define extra labels on serviceMonitor
  • allow toleration configuration (In my case, I want to exclude the master as it is a VM not compatible with scaphandre)

@damienvergnaud does this PR fit your needs ?

@bpetit in #230 the serviceMonitor.interval is set by default to 1m, this may be too hight. In our deployment I changed it to 5 minutes else scaphandre took too much CPU. Do you think, that default value should be changed ?

@damienvergnaud
Copy link

Hey @mmadoo,

It seems to go well visually speaking 👍 :

helm template helm/scaphandre/ -s templates/servicemonitor.yaml --api-versions monitoring.coreos.com/v1
---
# Source: scaphandre/templates/servicemonitor.yaml
apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  name: scaphandre-service-monitoring
  namespace: default
  labels:
    app.kubernetes.io/name: "scaphandre"
    app.kubernetes.io/managed-by: Helm
    helm.sh/chart: scaphandre-0.1.1
    release: prometheus-stack
spec:
  endpoints:
    - path: /metrics
      port: metrics
      scheme: http
      interval: 1m
      scrapeTimeout: 30s
  namespaceSelector:
    matchNames:
      - default
  selector:
    matchLabels:
      app.kubernetes.io/name: scaphandre

Questions about the PR :
If we trust enough this condition : ( .Capabilities.APIVersions.Has "monitoring.coreos.com/v1" ) why do we add a boolean to add the servicemonitor object at values.yaml level ?

Wouldn't it add simplicity to use lookup functions of helm to auto-inject scaphandre metrics inside if the cluster has the capability to do it ?
And transform this boolean to a "custom_config" var ?

Open to work on that with you if needs be.

@mmadoo
Copy link
Contributor Author

mmadoo commented Apr 25, 2023

If we trust enough this condition : ( .Capabilities.APIVersions.Has "monitoring.coreos.com/v1" ) why do we add a boolean to add the servicemonitor object at values.yaml level ?
I look on how bitnami chart are done.
I just set default value to false in order to have similar behaviour with previous version. Setting by default to true may be an acceptable breaking change.

@damienvergnaud
Copy link

It might require more thinking than passing it true by default, because if prometheus exporters doesn't "find" the service-monitor created by default at scaphandre installation by chart, the resource will just stand in the cluster for no reason.

Allow auto-integration of the scaphandre metrics to an existing prometheus installation might be another enhancement proposal.

@bpetit bpetit added this to Triage in General May 16, 2023
@mmadoo mmadoo force-pushed the helm/addLabelOnServiceMonitor branch from 3ff82c3 to 6595ebf Compare November 14, 2023 09:03
@mmadoo mmadoo force-pushed the helm/addLabelOnServiceMonitor branch from 6595ebf to a264c22 Compare February 13, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
General
Triage
Development

Successfully merging this pull request may close these issues.

None yet

2 participants