[stable/traefik] Decouple prometheus metrics to dashboard by mazzy89 · Pull Request #14946 · helm/charts

mazzy89 · 2019-06-20T18:54:05Z

What this PR does / why we need it:

Before this PR, in case the dashboard was not enabled, the Prometheus metrics were disabled.

It is now possible to enable Prometheus metrics having at the same time the dashboard disabled.

I've fully tested the PR locally and it works as expected.

Signed-off-by: Salvatore Mazzarino dev@mazzarino.cz

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

fixes #

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md
Title of the PR starts with chart name (e.g. [stable/chart])

k8s-ci-robot · 2019-06-20T18:54:19Z

Hi @mazzy89. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dtomcej · 2019-06-23T03:12:42Z

Hi @mazzy89, I like where this PR is going, but can you add a metrics.useOwnEntrypoint flag for this to maintain backwards compatability?

Thanks!

mazzy89 · 2019-06-23T09:10:18Z

@dtomcej So if I have understood correctly, you are proposing to not change the ConfigMap and introduce the config of the entry point for Prometheus via flags, right?

stale · 2019-07-28T17:32:32Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

mazzy89 · 2019-08-07T10:13:27Z

If anyone could take a look, I'd really appreciate it. thank you 🙏

chelnak · 2019-09-16T10:39:36Z

Yeah it looks like they are. It’s the killer with metrics and dashboard.

…

On Mon, 16 Sep 2019 at 11:10, Szymon Szypulski ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In stable/traefik/templates/service.yaml <#14946 (comment)>: > @@ -55,7 +55,7 @@ spec: targetPort: httpn {{- end }} {{- if (and (.Values.metrics.prometheus.enabled) (not (.Values.metrics.prometheus.restrictAccess)))}} - - port: 8080 Aren't all ports exported to the external ELB? I believe they were, when I tried to use it last time. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#14946?email_source=notifications&email_token=AA42IJUIJKQ6L4LMA6B4LWDQJ5LPRA5CNFSM4HZ2SVVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEZI7DQ#discussion_r324596028>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA42IJQO3P3II2ZJWRZUR6DQJ5LPRANCNFSM4HZ2SVVA> .

stale · 2019-10-16T11:32:06Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

zanhsieh · 2019-10-16T14:29:12Z

@mazzy89
Could you bump the chart to latest please? Thank you.

Before this PR, in case the dashboard was not enabled, the Prometheus metrics were disabled. It is now possible to enable Prometheus metrics having at the same time the dashboard disabled. Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz>

mazzy89 · 2019-10-26T13:40:19Z

@szamuboy Updated the PR by creating a separate Service for Prometheus metrics and @zanhsieh bumped the version here.

zanhsieh · 2019-10-26T19:20:08Z

/ok-to-test

mazzy89 · 2019-10-26T21:45:16Z

Thanks a lot @zanhsieh for the fast reaction ❤️

zanhsieh · 2019-10-26T22:08:45Z

@emilevauge
@dtomcej
@timoreimann
@vdemeester
@nmengin
@mmatur
@ldez
@juliens
@jbdoumenjou
@geraldcroes
@SantoDE
@errm
Would you guys LGTM this PR please? Thank you.

mazzy89 · 2019-10-28T17:06:03Z

🙏

vdemeester

/lgtm

k8s-ci-robot · 2019-10-29T09:19:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mazzy89, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~stable/traefik/OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

monotek · 2019-10-29T16:39:21Z

@mazzy89 & @vdemeester

This was a breaking change for us as all traefik metrics vanished after rollout :-(
Chart version should have been raised accordingly.

We had to change chart settings and switch from pod to service annotations.

Replace:

deployment:
  podAnnotations:
    prometheus.io/port: "8080"
    prometheus.io/scrape: "true"

With:

service:
  annotations:
    prometheus.io/port: "9100"
    prometheus.io/scrape: "true"

I also doubt that the pr made sense at all as we used metrics all the time but never used the dashboard.

errm · 2019-10-31T14:30:34Z

I also doubt that the pr made sense at all as we used metrics all the time but never used the dashboard.

If I understand it correctly this point here was to be able to expose metrics without exposing the dashboard ... which sounds exactly like your use-case

Before this PR, in case the dashboard was not enabled, the Prometheus metrics were disabled. It is now possible to enable Prometheus metrics having at the same time the dashboard disabled. Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz>

…18380) * [stable/traefik] Decouple prometheus metrics to dashboard (#14946) Before this PR, in case the dashboard was not enabled, the Prometheus metrics were disabled. It is now possible to enable Prometheus metrics having at the same time the dashboard disabled. Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz> Fix Fix for Jobs. Jobs also have `"helm.sh/hook": post-install` so they don't have this configmap after start. Pods won't start because of this. Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> Update Chart.yaml version up Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> fix Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * added expire to values Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * add ability to set expire Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * remove blank line Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * Bump version Co-Authored-By: Cédric de Saint Martin <cedric@desaintmartin.fr> Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * Added new conf params to Readme Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com>

…elm#18380) * [stable/traefik] Decouple prometheus metrics to dashboard (helm#14946) Before this PR, in case the dashboard was not enabled, the Prometheus metrics were disabled. It is now possible to enable Prometheus metrics having at the same time the dashboard disabled. Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz> Fix Fix for Jobs. Jobs also have `"helm.sh/hook": post-install` so they don't have this configmap after start. Pods won't start because of this. Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> Update Chart.yaml version up Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> fix Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * added expire to values Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * add ability to set expire Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * remove blank line Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * Bump version Co-Authored-By: Cédric de Saint Martin <cedric@desaintmartin.fr> Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * Added new conf params to Readme Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com>

Before this PR, in case the dashboard was not enabled, the Prometheus metrics were disabled. It is now possible to enable Prometheus metrics having at the same time the dashboard disabled. Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz>

…elm#18380) * [stable/traefik] Decouple prometheus metrics to dashboard (helm#14946) Before this PR, in case the dashboard was not enabled, the Prometheus metrics were disabled. It is now possible to enable Prometheus metrics having at the same time the dashboard disabled. Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz> Fix Fix for Jobs. Jobs also have `"helm.sh/hook": post-install` so they don't have this configmap after start. Pods won't start because of this. Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> Update Chart.yaml version up Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> fix Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * added expire to values Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * add ability to set expire Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * remove blank line Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * Bump version Co-Authored-By: Cédric de Saint Martin <cedric@desaintmartin.fr> Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com> * Added new conf params to Readme Signed-off-by: Nikita Massalitin <nmassalitin@pushwoosh.com>

rehevkor5 · 2020-04-14T19:57:10Z

@monotek Wont scraping metrics from the Service only give you one random Pod's metrics each time? Wouldn't this change be better?

deployment:
  podAnnotations:
    prometheus.io/scrape: "true"
    prometheus.io/port: "9100"

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 20, 2019

k8s-ci-robot requested review from juliens and nmengin June 20, 2019 18:54

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 20, 2019

mazzy89 mentioned this pull request Jun 20, 2019

[stable/traefik] Metrics cannot be enabled without dashboard #9672

Closed

mazzy89 force-pushed the traefik-metrics branch 2 times, most recently from db5b9a3 to 6d06f37 Compare June 28, 2019 17:01

stale Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 28, 2019

stale Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2019

szymonpk reviewed Aug 20, 2019

View reviewed changes

Comment thread stable/traefik/templates/service.yaml

mazzy89 force-pushed the traefik-metrics branch from d00b0bb to eedd88f Compare September 3, 2019 05:59

stale Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 16, 2019

stale Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 16, 2019

mazzy89 force-pushed the traefik-metrics branch from 26bc81f to 8e17252 Compare October 26, 2019 13:16

mazzy89 force-pushed the traefik-metrics branch from 8e17252 to 0ca59d8 Compare October 26, 2019 13:37

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 26, 2019

k8s-ci-robot added the ok-to-test label Oct 26, 2019

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 26, 2019

vdemeester reviewed Oct 29, 2019

View reviewed changes

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Oct 29, 2019

k8s-ci-robot merged commit 8e4efcd into helm:master Oct 29, 2019

Conversation

mazzy89 commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

Uh oh!

k8s-ci-robot commented Jun 20, 2019

Uh oh!

dtomcej commented Jun 23, 2019

Uh oh!

mazzy89 commented Jun 23, 2019

Uh oh!

stale Bot commented Jul 28, 2019

Uh oh!

mazzy89 commented Aug 7, 2019

Uh oh!

Uh oh!

chelnak commented Sep 16, 2019 via email

Uh oh!

stale Bot commented Oct 16, 2019

Uh oh!

zanhsieh commented Oct 16, 2019

Uh oh!

mazzy89 commented Oct 26, 2019

Uh oh!

zanhsieh commented Oct 26, 2019

Uh oh!

mazzy89 commented Oct 26, 2019

Uh oh!

zanhsieh commented Oct 26, 2019

Uh oh!

mazzy89 commented Oct 28, 2019

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 29, 2019

Uh oh!

monotek commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

errm commented Oct 31, 2019

Uh oh!

rehevkor5 commented Apr 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

mazzy89 commented Jun 20, 2019 •

edited

Loading

monotek commented Oct 29, 2019 •

edited

Loading