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

[stable/prometheus-blackbox-exporter] Fix blackbox target's metrics labels #22546

Merged
merged 1 commit into from
Jul 27, 2020
Merged

[stable/prometheus-blackbox-exporter] Fix blackbox target's metrics labels #22546

merged 1 commit into from
Jul 27, 2020

Conversation

mcanevet
Copy link
Contributor

@mcanevet mcanevet commented May 27, 2020

The metrics labels currently generated does not match what is generated
by the setup described here: https://prometheus.io/docs/guides/multi-target-exporter/#querying-multi-target-exporters-with-prometheus
and https://github.com/prometheus/blackbox_exporter#prometheus-configuration

The previous metricsRelabeling is only valid if the target contains the URLs to
check.

What we need in the end is:

  • __address__ set to the blackbox exporter, and it it already the case
    as the selector matches the blackbox exporter,
  • __param_target set to the target to check, and it is already ensure
    without relabeling,
  • instance set to the target to check.

This patches ensures that the metrics has this labels:

probe_success{endpoint="http",instance="http://prometheus.io",job="blackbox-exporter-prometheus-blackbox-exporter",namespace="monitoring",pod="blackbox-exporter-prometheus-blackbox-exporter-84d9b64646-vgqcc",service="blackbox-exporter-prometheus-blackbox-exporter",target="prometheus-io"} 1

Without this patch, we have this (note that we lost the instance label):

probe_success{endpoint="http",job="blackbox-exporter-prometheus-blackbox-exporter",namespace="monitoring",pod="blackbox-exporter-prometheus-blackbox-exporter-84d9b64646-vgqcc",service="blackbox-exporter-prometheus-blackbox-exporter",target="prometheus-io"} 1

Signed-off-by: Mickaël Canévet mickael.canevet@gmail.com

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/mychartname])

@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label May 27, 2020
@helm-bot helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 27, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @mcanevet. 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.

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.

@k8s-ci-robot 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 May 27, 2020
@zanhsieh
Copy link
Collaborator

zanhsieh commented Jun 7, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 7, 2020
@stale
Copy link

stale bot commented Jul 7, 2020

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 Jul 7, 2020
@desaintmartin
Copy link
Collaborator

Sorry, I didn't have time to investigate but this looks interesting, I'll try to find time to review/test.
/assign

@stale stale bot removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 7, 2020
@desaintmartin
Copy link
Collaborator

Okay, I've read a bit more about that and you seem to be right, however doing so would remove the "human readable name" that was set to the target label. Could we keep target but also add instance?

@mcanevet
Copy link
Contributor Author

@desaintmartin I updated my PR to keep the target label.

@desaintmartin
Copy link
Collaborator

Thanks! One last thing, could you fill the "how to upgrade" paragraph for the 5.0.0?

@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 21, 2020
@mcanevet
Copy link
Contributor Author

@desaintmartin done!

@helm-bot helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 27, 2020
The metrics labels currently generated does not match what is generated
by the setup described here: https://prometheus.io/docs/guides/multi-target-exporter/#querying-multi-target-exporters-with-prometheus
and https://github.com/prometheus/blackbox_exporter#prometheus-configuration

The previous metricsRelabeling is only valid if the target contains the URLs to
check.

What we need in the end is:
- `__address__` set to the blackbox exporter, and it it already the case
as the `selector` matches the blackbox exporter,
- `__param_target` set to the target to check, and it is already ensure
without relabeling,
- `instance` set to the target to check.

This patches ensures that the metrics has this labels:

```
probe_success{endpoint="http",instance="http://prometheus.io",job="blackbox-exporter-prometheus-blackbox-exporter",namespace="monitoring",pod="blackbox-exporter-prometheus-blackbox-exporter-84d9b64646-vgqcc",service="blackbox-exporter-prometheus-blackbox-exporter"} 1
```

Without this patch, we have this (note that we lost the `instance` label):

```
probe_success{endpoint="http",job="blackbox-exporter-prometheus-blackbox-exporter",namespace="monitoring",pod="blackbox-exporter-prometheus-blackbox-exporter-84d9b64646-vgqcc",service="blackbox-exporter-prometheus-blackbox-exporter",target="prometheus-io"} 1
```

Signed-off-by: Mickaël Canévet <mickael.canevet@gmail.com>
@desaintmartin
Copy link
Collaborator

Thanks a lot!
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: desaintmartin, mcanevet

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 Jul 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit 085678d into helm:master Jul 27, 2020
@mcanevet mcanevet deleted the fix_blackbox_labels branch July 27, 2020 19:49
li-adrienloiseau pushed a commit to li-adrienloiseau/charts that referenced this pull request Jul 29, 2020
The metrics labels currently generated does not match what is generated
by the setup described here: https://prometheus.io/docs/guides/multi-target-exporter/#querying-multi-target-exporters-with-prometheus
and https://github.com/prometheus/blackbox_exporter#prometheus-configuration

The previous metricsRelabeling is only valid if the target contains the URLs to
check.

What we need in the end is:
- `__address__` set to the blackbox exporter, and it it already the case
as the `selector` matches the blackbox exporter,
- `__param_target` set to the target to check, and it is already ensure
without relabeling,
- `instance` set to the target to check.

This patches ensures that the metrics has this labels:

```
probe_success{endpoint="http",instance="http://prometheus.io",job="blackbox-exporter-prometheus-blackbox-exporter",namespace="monitoring",pod="blackbox-exporter-prometheus-blackbox-exporter-84d9b64646-vgqcc",service="blackbox-exporter-prometheus-blackbox-exporter"} 1
```

Without this patch, we have this (note that we lost the `instance` label):

```
probe_success{endpoint="http",job="blackbox-exporter-prometheus-blackbox-exporter",namespace="monitoring",pod="blackbox-exporter-prometheus-blackbox-exporter-84d9b64646-vgqcc",service="blackbox-exporter-prometheus-blackbox-exporter",target="prometheus-io"} 1
```

Signed-off-by: Mickaël Canévet <mickael.canevet@gmail.com>
Signed-off-by: Adrien Loiseau <adrien.loiseau@logic-immo.com>
mmingorance-dh pushed a commit to mmingorance-dh/charts that referenced this pull request Aug 28, 2020
The metrics labels currently generated does not match what is generated
by the setup described here: https://prometheus.io/docs/guides/multi-target-exporter/#querying-multi-target-exporters-with-prometheus
and https://github.com/prometheus/blackbox_exporter#prometheus-configuration

The previous metricsRelabeling is only valid if the target contains the URLs to
check.

What we need in the end is:
- `__address__` set to the blackbox exporter, and it it already the case
as the `selector` matches the blackbox exporter,
- `__param_target` set to the target to check, and it is already ensure
without relabeling,
- `instance` set to the target to check.

This patches ensures that the metrics has this labels:

```
probe_success{endpoint="http",instance="http://prometheus.io",job="blackbox-exporter-prometheus-blackbox-exporter",namespace="monitoring",pod="blackbox-exporter-prometheus-blackbox-exporter-84d9b64646-vgqcc",service="blackbox-exporter-prometheus-blackbox-exporter"} 1
```

Without this patch, we have this (note that we lost the `instance` label):

```
probe_success{endpoint="http",job="blackbox-exporter-prometheus-blackbox-exporter",namespace="monitoring",pod="blackbox-exporter-prometheus-blackbox-exporter-84d9b64646-vgqcc",service="blackbox-exporter-prometheus-blackbox-exporter",target="prometheus-io"} 1
```

Signed-off-by: Mickaël Canévet <mickael.canevet@gmail.com>
Signed-off-by: Miguel Mingorance <miguel.mingorance@deliveryhero.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. ok-to-test size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants