Feat/add blackbox probes library #43

giusebar · 2024-09-18T09:52:58Z

Issue

Closes #42.

Solution

Draft pull request adding a blackbox_probes library, to allow client charm to provide blackbox Exporter with some endpoints to monitor.

lib/charms/blackbox_k8s/v0/blackbox_probes.py

Abuelodelanada · 2024-10-10T15:40:51Z

lib/charms/blackbox_k8s/v0/blackbox_probes.py

+from pydantic import BaseModel, ConfigDict, Field, Json
+
+# The unique Charmhub library identifier, never change it
+LIBID = "1"


I think you need to run charmcraft create-lib in order to get the unique identifier assigned by Charmhub

I'm getting:

Store operation failed: - permission-required: No publisher or collaborator permission for the blackbox-exporter-k8s charm package

lib/charms/blackbox_k8s/v0/blackbox_probes.py

Abuelodelanada

In general LGTM, but I would change some minor things

sed-i

Thank you for this powerful contribution!
Mostly non-code comments.

sed-i · 2024-10-16T03:38:13Z

metadata.yaml

@@ -47,3 +47,5 @@ requires:
    limit: 1
  catalogue:
    interface: catalogue
+  blackbox-probes:
+    interface: blackbox_probes


Suggested change

interface: blackbox_probes

interface: blackbox_probes

description: |

Receive a list of probes (server address and probing method) from the provider to query.

sed-i · 2024-10-16T03:40:03Z

metadata.yaml

@@ -47,3 +47,5 @@ requires:
    limit: 1
  catalogue:
    interface: catalogue
+  blackbox-probes:


Should the relation name be just probes here on the requirer side?

By convention, interface names should be unique in the ecosystem. Is "blackbox-probes" specific enough? Should we name it "blackbox_exporter_probes"?

makes sense, changing it.

sed-i · 2024-10-16T03:47:34Z

README.md

+#### Dynamic Configuration
+
+The list of probes and the list of modules for probing can also be changed dynamically from other charms.
+The charm offers a relation to allow charms to dynamically export endpoints to be probed via Blackbox and custom modules for probing. Those are exported over the blackbox-targets relation using the blackbox_probes interface:


Should we stick to the official product name?

Suggested change

The charm offers a relation to allow charms to dynamically export endpoints to be probed via Blackbox and custom modules for probing. Those are exported over the blackbox-targets relation using the blackbox_probes interface:

The charm offers a relation to allow charms to dynamically export endpoints to be probed via Blackbox Exporter and custom modules for probing. Those are exported over the blackbox-probes relation using the blackbox_probes interface:

sed-i · 2024-10-16T03:50:23Z

README.md

+In order for the charm defined probes to be probed via this charm all that is required is to relate the two charms with:
+
+```shell
+juju relate <charm> blackbox


We may want to be a bit more prescriptive, to avoid ambiguity:

Suggested change

juju relate <charm> blackbox

juju relate <charm> blackbox:probes

sed-i · 2024-10-16T03:51:02Z

README.md

+juju relate <charm> blackbox
+```
+
+Charms that seek to provide probes for Blackbox, must do so using the provided blackbox_probes charm library. This library ensures that probes and modules defined by a charm are forwarded correctly to Prometheus, and the metrics displayed in the associated Grafana Dashboard.


Suggested change

Charms that seek to provide probes for Blackbox, must do so using the provided blackbox_probes charm library. This library ensures that probes and modules defined by a charm are forwarded correctly to Prometheus, and the metrics displayed in the associated Grafana Dashboard.

Charms that seek to provide probes for Blackbox Exporter, must do so using the provided blackbox_probes charm library. This library ensures that probes and modules defined by a charm are forwarded correctly to Prometheus, and the metrics displayed in the associated Grafana Dashboard.

sed-i · 2024-10-16T04:45:18Z

lib/charms/blackbox_k8s/v0/blackbox_probes.py

+## Provider Library Usage
+
+The Blackbox Exporter charm interacts with its datasources using this charm
+library. The goal of this library is to be as simple to use as possible.


Suggested change

library. The goal of this library is to be as simple to use as possible.

library.

sed-i · 2024-10-16T04:46:20Z

lib/charms/blackbox_k8s/v0/blackbox_probes.py

+
+The Blackbox Exporter charm interacts with its datasources using this charm
+library. The goal of this library is to be as simple to use as possible.
+Charms seeking to expose metric endpoints to be probed via Blackbox, must do so


Afaiu, there are no metric endpoints to begin with, which is why we're using blackbox exporter (?).

sed-i · 2024-10-16T04:47:11Z

lib/charms/blackbox_k8s/v0/blackbox_probes.py

+is exposed to the Blakcbox Exporter charm. This relation must use the
+`blackbox_probes` interface.
+The default name for the metrics endpoint relation is
+`blackbox-targets`. It is strongly recommended to use the same


Suggested change

`blackbox-targets`. It is strongly recommended to use the same

`blackbox-probes`. It is strongly recommended to use the same

sed-i · 2024-10-16T04:48:15Z

lib/charms/blackbox_k8s/v0/blackbox_probes.py

+    def __init__(self, *args):
+        super().__init__(*args)
+        ...
+        self.probes_provider = BlackboxProbesProvider(self, probes=probes_endpoints_config)


Is there a simple config so short that we could inline it in this example, to better illustrate?

sed-i · 2024-10-16T04:49:46Z

lib/charms/blackbox_k8s/v0/blackbox_probes.py

+An instantiated `BlackboxProbesProvider` object will ensure that
+the list of endpoints to be probed are passed through to Blackbox Exporter,
+which will format them to be scraped for Prometheus.


Should we rephrase this?

sed-i · 2024-10-17T12:53:26Z

Could you please also link to a tandem PR with the provider side of things? Was it supposed to be https://github.com/ubuntu-robotics/cos-registration-server?
We would like to be able to test this manually.

giusebar · 2024-10-17T14:17:42Z

Could you please also link to a tandem PR with the provider side of things? Was it supposed to be https://github.com/ubuntu-robotics/cos-registration-server? We would like to be able to test this manually.

Hello @sed-i, thanks for the review. I'll try to address the comments today and tomorrow. In the meantime, this is the PR with the provider side of things: canonical/cos-registration-server-k8s-operator#13.

Just as a reminder, this library PR includes also the changes proposed in PR #41. I've seen you made some comments about those changes. I had to incorporate them because without them I could not properly test blackbox (prometheus would return some errors). I can confirm that with those changes the traefik links work as expected as well as the probes, but if you could double test it it would be great.

sed-i · 2024-11-22T19:59:33Z

README.md

+#### Dynamic Configuration
+
+The list of probes and the list of modules for probing can also be changed dynamically from other charms.
+The charm offers a relation to allow charms to dynamically export endpoints to be probed via Blackbox Exporter and custom modules for probing. Those are exported over the probes relation using the blackbox_exporter_probes interface:


Suggested change

The charm offers a relation to allow charms to dynamically export endpoints to be probed via Blackbox Exporter and custom modules for probing. Those are exported over the probes relation using the blackbox_exporter_probes interface:

The charm offers a relation to allow charms to forward custom probe spec to Blackbox Exporter. Those are exported over the probes relation using the blackbox_exporter_probes interface:

sed-i · 2024-11-22T20:01:23Z

README.md

+    interface: blackbox_exporter_probes
+```
+
+The probes provided dynamically by a charm are merged with the probes defined in a configuration file, same with the modules which are integrated in the blackbox-config file.


Suggested change

The probes provided dynamically by a charm are merged with the probes defined in a configuration file, same with the modules which are integrated in the blackbox-config file.

The cusrom probes provided via relation data are merged with the probes defined in a configuration file, same with the modules which are integrated in the blackbox-config file.

sed-i · 2024-11-22T20:02:48Z

README.md

+juju relate <charm> blackbox:probes
+```
+
+Charms that seek to provide probes for Blackbox Exporter, must do so using the provided blackbox_exporter_probes charm library. This library ensures that probes and modules defined by a charm are forwarded correctly to Prometheus, and the metrics displayed in the associated Grafana Dashboard.


Suggested change

Charms that seek to provide probes for Blackbox Exporter, must do so using the provided blackbox_exporter_probes charm library. This library ensures that probes and modules defined by a charm are forwarded correctly to Prometheus, and the metrics displayed in the associated Grafana Dashboard.

Charms that seek to provide probes for Blackbox Exporter, can do so using the provided blackbox_exporter_probes charm library. This library ensures that probes and modules defined by a charm are forwarded correctly to Prometheus, and the metrics displayed in the associated Grafana Dashboard.

sed-i · 2024-11-22T20:07:23Z

lib/charms/blackbox_k8s/v0/blackbox_probes.py

+
+The Blackbox Exporter charm interacts with its datasources using this charm
+library.
+Charms seeking to expose probes for Blackbox, must do so


Suggested change

Charms seeking to expose probes for Blackbox, must do so

Charms seeking to expose probes for Blackbox, may do so

sed-i · 2024-11-22T20:14:21Z

lib/charms/blackbox_k8s/v0/blackbox_probes.py

+1. Instantiate the `BlackboxProbesRequirer` object by providing it a
+reference to the parent (Blackbox Exporter) charm and, optionally, the name of
+the relation that the Blackbox Exporter charm uses to interact with probes
+targets. This relation must confirm to the `blackbox_exporter_probes`


Suggested change

targets. This relation must confirm to the `blackbox_exporter_probes`

targets. This relation must conform to the `blackbox_exporter_probes`

sed-i · 2024-11-22T20:59:27Z

src/charm.py

+    def _merge_scrape_configs(
+        self, file_probes: dict, relation_probes: List[ProbesJobModel]
+    ) -> List[dict]:
+        """Merge the scrape_configs from both file and relation.


Could you please add to docstring the order of precedence explicitly?

sed-i · 2024-11-22T21:00:39Z

src/charm.py

+        self.container.push(self._config_path, updated_config_data)
+        self.blackbox_workload.reload()
+
+    def _merge_scrape_configs(


Would it make sense to extract this logic outside of charm code?

sed-i · 2024-11-22T21:02:11Z

src/charm.py

+        for probe in relation_probes:
+            if not isinstance(probe, dict):
+                probe = dict(probe)
+            job_name = probe["job_name"]


I think name collisions would be accidental, i.e. representing different probes.
Should we differentiate by adding rel-id to the name?

sed-i · 2024-11-22T21:02:57Z

src/charm.py

+            probe["metrics_path"] = probes_path
+            # The relabel configs come from the official Blackbox Exporter docs; please refer
+            # to that for further information on what they do
+            probe["relabel_configs"] = [


Should we extract this into a charm-independent config builder?

sed-i · 2024-11-22T21:05:58Z

tests/unit/test_probes_provider.py

+        rel_id = self.harness.add_relation(RELATION_NAME, "provider")
+        self.harness.add_relation_unit(rel_id, "provider/0")
+        self.harness.charm.provider.set_probes_spec()
+        self.assertEqual(self.harness.charm._stored.num_events, 2)


This kind of whitebox testing tends to be brittle and difficult to maintain.
See BDD section here.

simskij · 2024-12-03T13:41:36Z

@giusebar, sorry to bother you, but do you think you have a moment to look at this in the coming weeks? Thanks in advance.

…parate module, use centralised collect_status in the blackbox-library, improve testing

… well

giusebar added 5 commits September 11, 2024 15:14

wip - add library and use it in blackbox

7f0fddb

pass full configuration in the relation

976981b

add library documentation, add monitor targets_changed in the charm

f6dcd4e

add provider and consumer tests

786db08

update config file with relation modules correctly

5519d53

giusebar force-pushed the feat/add-blackbox-probes-library branch from 50d4e33 to 5519d53 Compare September 19, 2024 16:58

add pydantic validation for probes and modules

c267fcc

giusebar marked this pull request as ready for review September 26, 2024 08:26

giusebar requested a review from a team as a code owner September 26, 2024 08:26

giusebar force-pushed the feat/add-blackbox-probes-library branch from 2672b6c to d909f66 Compare October 1, 2024 11:06

add pydantic check on the requirer side, small fixes

b08a263

giusebar force-pushed the feat/add-blackbox-probes-library branch from d909f66 to b08a263 Compare October 3, 2024 17:20

giusebar mentioned this pull request Oct 7, 2024

Add integration with blackbox for self-monitoring canonical/ros2bag-fileserver-k8s-operator#8

Open

sed-i mentioned this pull request Oct 7, 2024

cert expiration monitoring canonical/traefik-k8s-operator#380

Closed

update README

ce8e4df

giusebar force-pushed the feat/add-blackbox-probes-library branch from ca48af8 to ce8e4df Compare October 8, 2024 14:17

remove unnecessary patch network get

eff4d9a

giusebar force-pushed the feat/add-blackbox-probes-library branch from 18fd730 to eff4d9a Compare October 8, 2024 14:32

Abuelodelanada reviewed Oct 10, 2024

View reviewed changes

sed-i reviewed Oct 16, 2024

View reviewed changes

giusebar added 4 commits November 12, 2024 14:22

address comments

ae10472

Merge branch 'main' into feat/add-blackbox-probes-library

c2c11d3

fix docstring and lint

e6b28c7

add lib unique id

001fc6a

sed-i reviewed Nov 22, 2024

View reviewed changes

Merge branch 'main' into feat/add-blackbox-probes-library

bdfdd8d

giusebar added 5 commits December 11, 2024 10:41

extract blackbox probes configuration build from charm code into a se…

9bcc2a9

…parate module, use centralised collect_status in the blackbox-library, improve testing

fix lint

6601b60

remove invalid_probe custom event and use stored state for modules as…

3fbdb94

… well

add hash for ensure probes uniqueness, add test

be3701e

remove outdated comment

693ce7d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/add blackbox probes library #43

Feat/add blackbox probes library #43

giusebar commented Sep 18, 2024 •

edited by lucabello

Loading

Abuelodelanada Oct 10, 2024

giusebar Nov 12, 2024

Abuelodelanada left a comment

sed-i left a comment

sed-i Oct 16, 2024

sed-i Oct 16, 2024

giusebar Oct 18, 2024

sed-i Oct 16, 2024

sed-i Oct 16, 2024

sed-i Oct 16, 2024

sed-i Oct 16, 2024

sed-i Oct 16, 2024

sed-i Oct 16, 2024

sed-i Oct 16, 2024

sed-i Oct 16, 2024

sed-i commented Oct 17, 2024

giusebar commented Oct 17, 2024 •

edited

Loading

sed-i Nov 22, 2024

sed-i Nov 22, 2024

sed-i Nov 22, 2024

sed-i Nov 22, 2024

sed-i Nov 22, 2024

sed-i Nov 22, 2024

sed-i Nov 22, 2024

sed-i Nov 22, 2024

sed-i Nov 22, 2024

sed-i Nov 22, 2024

simskij commented Dec 3, 2024

	The charm offers a relation to allow charms to dynamically export endpoints to be probed via Blackbox and custom modules for probing. Those are exported over the blackbox-targets relation using the blackbox_probes interface:
	The charm offers a relation to allow charms to dynamically export endpoints to be probed via Blackbox Exporter and custom modules for probing. Those are exported over the blackbox-probes relation using the blackbox_probes interface:

	juju relate <charm> blackbox
	juju relate <charm> blackbox:probes

	Charms that seek to provide probes for Blackbox, must do so using the provided blackbox_probes charm library. This library ensures that probes and modules defined by a charm are forwarded correctly to Prometheus, and the metrics displayed in the associated Grafana Dashboard.
	Charms that seek to provide probes for Blackbox Exporter, must do so using the provided blackbox_probes charm library. This library ensures that probes and modules defined by a charm are forwarded correctly to Prometheus, and the metrics displayed in the associated Grafana Dashboard.

	library. The goal of this library is to be as simple to use as possible.
	library.

	`blackbox-targets`. It is strongly recommended to use the same
	`blackbox-probes`. It is strongly recommended to use the same

	The probes provided dynamically by a charm are merged with the probes defined in a configuration file, same with the modules which are integrated in the blackbox-config file.
	The cusrom probes provided via relation data are merged with the probes defined in a configuration file, same with the modules which are integrated in the blackbox-config file.

	Charms seeking to expose probes for Blackbox, must do so
	Charms seeking to expose probes for Blackbox, may do so

	targets. This relation must confirm to the `blackbox_exporter_probes`
	targets. This relation must conform to the `blackbox_exporter_probes`

Feat/add blackbox probes library #43

Are you sure you want to change the base?

Feat/add blackbox probes library #43

Conversation

giusebar commented Sep 18, 2024 • edited by lucabello Loading

Issue

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abuelodelanada left a comment

Choose a reason for hiding this comment

sed-i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sed-i commented Oct 17, 2024

giusebar commented Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simskij commented Dec 3, 2024

giusebar commented Sep 18, 2024 •

edited by lucabello

Loading

giusebar commented Oct 17, 2024 •

edited

Loading