Skip to content
This repository was archived by the owner on Feb 22, 2022. It is now read-only.

[stable/concourse] Moves prometheus annotation to deployment #9536

Merged
merged 3 commits into from
Jan 14, 2019

Conversation

cirocosta
Copy link
Collaborator

What this PR does / why we need it:

Previously, Prometheus annotations were added to the web service,
which is meant to represent the whole replica set of pods, leaving
no pod-specific labels to the metrics gathered, defeating the
purpose of some of them (that are meant to be container-specific).

By moving them to deployment, we're able to collect them per-pod,
instead of per-service.

Which issue this PR fixes

Special notes for your reviewer:

Although it was possible to still have the effects of this change working without making the annotation per-deployment (by not setting the concourse.web.prometheus.enabled flag and manually including the annotations + env variables), I'm confident that going this route makes more sense.

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

cc @frodenas / @william-tran

@helm-bot 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 Nov 25, 2018
@stale
Copy link

stale bot commented Dec 26, 2018

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.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 26, 2018
Previously, Prometheus annotations were added to the web service,
which is meant to represent the whole replica set of pods, leaving
no pod-specific labels to the metrics gathered, defeating the
purpose of some of them (that are meant to be container-specific).

By moving them to `deployment`, we're able to collect them per-pod,
instead of per-service.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
@cirocosta cirocosta force-pushed the concourse-prometheus-scrape-pods branch from a28bf80 to 2636270 Compare January 6, 2019 03:54
@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 6, 2019
@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 6, 2019
@william-tran
Copy link
Collaborator

william-tran commented Jan 14, 2019

From https://groups.google.com/forum/#!topic/prometheus-users/jDEzVHUUFzw it looks like prometheus scrapes each pod behind the service, and from #6338 and #6422 it looks like it is desirable to have it on the service to support prometheus-operator. Not having used that myself, can you add anything to that @2color @richardalberto?

@william-tran
Copy link
Collaborator

But, @cirocosta if you've observed differences in pod scraping vs service scraping that obfuscate some metrics, please add that to the discussion

@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 14, 2019
@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 14, 2019
@paulczar
Copy link
Collaborator

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cirocosta, paulczar

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 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 Jan 14, 2019
@k8s-ci-robot k8s-ci-robot merged commit ff86c68 into helm:master Jan 14, 2019
@william-tran
Copy link
Collaborator

@paulczar I do not appreciate you lgtm-ing while this PR is being discussed. Why did you do that?

cirocosta pushed a commit to cirocosta/charts that referenced this pull request Jan 14, 2019
…elm#9536)"

This reverts commit ff86c68.

The change was still being discussed when this got merged.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
cirocosta pushed a commit to cirocosta/charts that referenced this pull request Jan 14, 2019
…elm#9536)"

This reverts commit ff86c68.

The change was still being discussed when this got merged.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
@paulczar
Copy link
Collaborator

sorry about that, as you know there was a backlog of PRs for the concourse chart, I was trying to clear through a bunch that were approved but had version conflicts as i could fix the conflicts through the web ui. Clearly I either missed the ongoing discussion here, Will figure out on slack with you if we want to revert or leave as is.

@william-tran
Copy link
Collaborator

To continue this discussion; I think we need to keep the labels where they are; though prometheus-operator can use other mechanisms for discovering things to scrape, I assume there might be some configurations out there that use these service level annotations. This assumption should be true considering that was the original reason why they were added to the service, but again I'd like actual users @2color @richardalberto to chime in.

cirocosta pushed a commit to cirocosta/charts that referenced this pull request Jan 15, 2019
…elm#9536)"

This reverts commit ff86c68.

The change was still being discussed when this got merged.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
k8s-ci-robot pushed a commit that referenced this pull request Jan 15, 2019
…9536)" (#10624)

This reverts commit ff86c68.

The change was still being discussed when this got merged.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
Previously, Prometheus annotations were added to the web service,
which is meant to represent the whole replica set of pods, leaving
no pod-specific labels to the metrics gathered, defeating the
purpose of some of them (that are meant to be container-specific).

By moving them to `deployment`, we're able to collect them per-pod,
instead of per-service.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
…elm#9536)" (helm#10624)

This reverts commit ff86c68.

The change was still being discussed when this got merged.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
hakamadare pushed a commit to hakamadare/charts that referenced this pull request Jan 29, 2019
Previously, Prometheus annotations were added to the web service,
which is meant to represent the whole replica set of pods, leaving
no pod-specific labels to the metrics gathered, defeating the
purpose of some of them (that are meant to be container-specific).

By moving them to `deployment`, we're able to collect them per-pod,
instead of per-service.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
hakamadare pushed a commit to hakamadare/charts that referenced this pull request Jan 29, 2019
…elm#9536)" (helm#10624)

This reverts commit ff86c68.

The change was still being discussed when this got merged.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
hakamadare pushed a commit to hakamadare/charts that referenced this pull request Jan 29, 2019
Previously, Prometheus annotations were added to the web service,
which is meant to represent the whole replica set of pods, leaving
no pod-specific labels to the metrics gathered, defeating the
purpose of some of them (that are meant to be container-specific).

By moving them to `deployment`, we're able to collect them per-pod,
instead of per-service.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
hakamadare pushed a commit to hakamadare/charts that referenced this pull request Jan 29, 2019
…elm#9536)" (helm#10624)

This reverts commit ff86c68.

The change was still being discussed when this got merged.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants