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

[helm] Add support for relabelings and metricRelabelings at serviceMonitor #3366

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Feb 4, 2023

Description

I need to add some additional labels to all metrics.

Fixes #ISSUE

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 4, 2023
@jkroepke jkroepke changed the title [helm] Add support for relabelings and metricRelabelings at serviceMo… [helm] Add support for relabelings and metricRelabelings at serviceMonitor Feb 4, 2023
@jkroepke jkroepke force-pushed the relabeling branch 2 times, most recently from 78d205f to 82f8e04 Compare February 6, 2023 19:24
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2023
@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 6, 2023

@stevehipwell I would appreciate any feedback here.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 6, 2023
Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

I think it might be best to reduce the tight coupling with the ServiceMonitor spec and provide a more flexible solution where we don't need to add all the fields into the values?

serviceMonitor:
  enabled: false
  namespace:
  additionalLabels: {}
  annotations: {}
  targetLabels: []
  endpointConfig:
    interval: 1m
    scrapeTimeout: 10s

Then if you wanted to template this you could use a string as so.

serviceMonitor:
  enabled: false
  namespace:
  additionalLabels: {}
  annotations: {}
  targetLabels: []
  endpointConfig: |-
    interval: 1m
    scrapeTimeout: 10s
    metricRelabelings:
      {{ .Values.test }}

charts/external-dns/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
charts/external-dns/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 7, 2023

I remove the tpl function. I do not have a use case for it rn, I copied it from prometheus helm chart.

I can implement the ServiceMonitor flexible approach with the endpointConfig. But it would be a really special way compared to other world known helm charts. So please re-confirm we if you want go the more generic approch.

I personally would prefer to follow the same pattern as other helm charts are doing this to keep a consistent chart experience

@jkroepke jkroepke requested review from stevehipwell and removed request for njuettner February 7, 2023 20:34
@stevehipwell
Copy link
Contributor

@jkroepke I think the problem here is twofold. A lot of the well known Helm charts don't have much of a submission gate which results in completely incorrect implementations such as tpl for an object and API designs which have grown organically rather than there having been though applied. This leads to a naïve implementation requiring the duplication of a resource spec in both the values and the template leading to a brittle chart. For my charts I use the endpointConfig pattern to save this issue, but I get that the other pattern has become pervasive. Let's keep your implementation for now and I'll look at using an omit pattern to remove the explicit tight coupling in the template.

@stevehipwell
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2023
@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 9, 2023

Thanks @stevehipwell , I'm looking forward on this PR

@stevehipwell
Copy link
Contributor

@Raffo could you approve this or do you want me to LGTM & approve these myself?

@Raffo
Copy link
Contributor

Raffo commented Feb 9, 2023

@stevehipwell I can approve this one, but I have zero problem with you doing it. Feel free to own help related changes.

@Raffo
Copy link
Contributor

Raffo commented Feb 9, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkroepke, Raffo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants