Fix scrape jobs so that there are no duplicate job names. (#292)

* Fix scrape jobs so that there are no duplicate job names. This is because duplicate job names cause errors in the prometheus config * formatting * only use multiple jobs for the tests that require it * Better wording Co-authored-by: Leon <[email protected]> * Bad comment Co-authored-by: Leon <[email protected]> * shorten comment for linter * make comments more clear * Type hint Co-authored-by: Leon <[email protected]> * ensure function does not modify argument * must be trusted in order to get volume size * use upstream-source Co-authored-by: Leon <[email protected]>
canonical · Jun 17, 2022 · 5f4d32d · 5f4d32d
1 parent 93e1e46
commit 5f4d32d
Show file tree

Hide file tree

Showing 6 changed files with 184 additions and 7 deletions.
diff --git a/lib/charms/prometheus_k8s/v0/prometheus_scrape.py b/lib/charms/prometheus_k8s/v0/prometheus_scrape.py
@@ -311,6 +311,8 @@ def _on_scrape_targets_changed(self, event):
 
 """  # noqa: W505
 
+import copy
+import hashlib
 import ipaddress
 import json
 import logging
@@ -881,6 +883,8 @@ def jobs(self) -> list:
             if static_scrape_jobs:
                 scrape_jobs.extend(static_scrape_jobs)
 
+        scrape_jobs = _dedupe_job_names(scrape_jobs)
+
         return scrape_jobs
 
     def alerts(self) -> dict:
@@ -1235,6 +1239,49 @@ def _labeled_unit_config(
         return static_config
 
 
+def _dedupe_job_names(jobs: List[dict]):
+    """Deduplicate a list of dicts by appending a hash to the value of the 'job_name' key.
+
+    Additionally fully dedeuplicate any identical jobs.
+
+    Args:
+        jobs: A list of prometheus scrape jobs
+    """
+    jobs_copy = copy.deepcopy(jobs)
+
+    # Convert to a dict with job names as keys
+    # I think this line is O(n^2) but it should be okay given the list sizes
+    jobs_dict = {
+        job["job_name"]: list(filter(lambda x: x["job_name"] == job["job_name"], jobs_copy))
+        for job in jobs_copy
+    }
+
+    # If multiple jobs have the same name, convert the name to "name_<hash-of-job>"
+    for key in jobs_dict:
+        if len(jobs_dict[key]) > 1:
+            for job in jobs_dict[key]:
+                job_json = json.dumps(job)
+                hashed = hashlib.sha256(job_json.encode()).hexdigest()
+                job["job_name"] = "{}_{}".format(job["job_name"], hashed)
+    new_jobs = []
+    for key in jobs_dict:
+        new_jobs.extend([i for i in jobs_dict[key]])
+
+    # Deduplicate jobs which are equal
+    # Again this in O(n^2) but it should be okay
+    deduped_jobs = []
+    seen = []
+    for job in new_jobs:
+        job_json = json.dumps(job)
+        hashed = hashlib.sha256(job_json.encode()).hexdigest()
+        if hashed in seen:
+            continue
+        seen.append(hashed)
+        deduped_jobs.append(job)
+
+    return deduped_jobs
+
+
 def _resolve_dir_against_charm_path(charm: CharmBase, *path_elements: str) -> str:
     """Resolve the provided path items against the directory of the main file.
 

diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py
@@ -82,7 +82,7 @@ async def get_prometheus_config(ops_test: OpsTest, app_name: str, unit_num: int)
 
 
 async def get_prometheus_active_targets(
-    ops_test: OpsTest, app_name: str, unit_num: int
+    ops_test: OpsTest, app_name: str, unit_num: int = 0
 ) -> List[dict]:
     """Fetch Prometheus active scrape targets.
 

diff --git a/tests/integration/prometheus-tester/config.yaml b/tests/integration/prometheus-tester/config.yaml
@@ -10,3 +10,7 @@ options:
     default: src/prometheus_alert_rules
     description: "Path for alert rules passed to the Provider"
     type: string
+  scrape_jobs:
+    default: "[]"
+    description: "Scrape jobs to pass to the MetricsEndpointProvider constructor"
+    type: string
diff --git a/tests/integration/prometheus-tester/src/charm.py b/tests/integration/prometheus-tester/src/charm.py
@@ -4,6 +4,7 @@
 
 """A Charm to functionally test the Prometheus Operator."""
 
+import json
 import logging
 from pathlib import Path
 
@@ -24,12 +25,13 @@ def __init__(self, *args):
         self._name = "prometheus-tester"
         self._pip_path = "/usr/local/bin/pip"
         self._metrics_exporter_script = Path("src/metrics.py")
-        jobs = [
-            {
-                "scrape_interval": self.model.config["scrape-interval"],
-                "static_configs": [{"targets": ["*:8000"], "labels": {"name": self._name}}],
-            }
-        ]
+        if not (jobs := json.loads(self.config["scrape_jobs"])):
+            jobs = [
+                {
+                    "scrape_interval": self.model.config["scrape-interval"],
+                    "static_configs": [{"targets": ["*:8000"], "labels": {"name": self._name}}],
+                }
+            ]
         logger.warning("Rules path is: %s", self.model.config["alert-rules-path"])
         self.prometheus = MetricsEndpointProvider(
             self, jobs=jobs, alert_rules_path=self.model.config["alert-rules-path"]

diff --git a/tests/integration/test_deduplication.py b/tests/integration/test_deduplication.py
@@ -0,0 +1,59 @@
+#!/usr/bin/env python3
+# Copyright 2022 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+import asyncio
+import json
+
+from helpers import get_prometheus_active_targets, oci_image
+from pytest_operator.plugin import OpsTest
+
+prometheus_app_name = "prometheus"
+prometheus_resources = {"prometheus-image": oci_image("./metadata.yaml", "prometheus-image")}
+tester_app_name = "tester"
+tester_resources = {
+    "prometheus-tester-image": oci_image(
+        "./tests/integration/prometheus-tester/metadata.yaml",
+        "prometheus-tester-image",
+    )
+}
+
+
+async def test_multiple_scrape_jobs_in_constructor(
+    ops_test: OpsTest, prometheus_charm, prometheus_tester_charm
+):
+    """Test that job names are properly deduped when in the same consumer unit."""
+    jobs = [
+        {
+            "scrape_interval": "10s",
+            "static_configs": [{"targets": ["*:8000"]}],
+        },
+        {
+            "scrape_interval": "10s",
+            "static_configs": [{"targets": ["*:8000"]}],
+        },
+        {
+            "scrape_interval": "10s",
+            "static_configs": [{"targets": ["*:8001"]}],
+        },
+    ]
+    await asyncio.gather(
+        ops_test.model.deploy(
+            prometheus_charm,
+            resources=prometheus_resources,
+            application_name=prometheus_app_name,
+            trust=True,
+        ),
+        ops_test.model.deploy(
+            prometheus_tester_charm,
+            resources=tester_resources,
+            application_name=tester_app_name,
+            config={"scrape_jobs": json.dumps(jobs)},
+        ),
+    )
+    await ops_test.model.add_relation(prometheus_app_name, tester_app_name)
+    await ops_test.model.wait_for_idle(status="active")
+
+    targets = await get_prometheus_active_targets(ops_test, prometheus_app_name)
+    # Two unique jobs above plus an additional an additional job for self scraping.
+    assert len(targets) == 3
diff --git a/tests/unit/test_functions.py b/tests/unit/test_functions.py
@@ -0,0 +1,65 @@
+# Copyright 2022 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+import copy
+import unittest
+
+import deepdiff
+from charms.prometheus_k8s.v0.prometheus_scrape import _dedupe_job_names
+
+
+class TestFunctions(unittest.TestCase):
+    def test_dedupe_job_names(self):
+        jobs = [
+            {
+                "job_name": "job0",
+                "static_configs": [{"targets": ["localhost:9090"]}],
+                "scrape_interval": "5s",
+            },
+            {
+                "job_name": "job0",
+                "static_configs": [{"targets": ["localhost:9090"]}],
+                "scrape_interval": "5s",
+            },
+            {
+                "job_name": "job1",
+                "static_configs": [{"targets": ["localhost:9090"]}],
+                "scrape_interval": "5s",
+            },
+            {
+                "job_name": "job0",
+                "static_configs": [{"targets": ["localhost:9090"]}],
+                "scrape_interval": "10s",
+            },
+            {
+                "job_name": "job0",
+                "static_configs": [{"targets": ["localhost:9091"]}],
+                "scrape_interval": "5s",
+            },
+        ]
+        jobs_original = copy.deepcopy(jobs)
+        expected = [
+            {
+                "job_name": "job0_6f9f1c305506707b952aef3885fa099fe36158f6359b8a06634068270645aefd",
+                "scrape_interval": "5s",
+                "static_configs": [{"targets": ["localhost:9090"]}],
+            },
+            {
+                "job_name": "job0_c651cf3a8cd1b85abc0cf7620e058b87ef43e2296d1520328ce5a796e9b20993",
+                "scrape_interval": "10s",
+                "static_configs": [{"targets": ["localhost:9090"]}],
+            },
+            {
+                "job_name": "job0_546b5bbb56e719d894b0a557975e0926ed093ea547c87051595d953122d2a7d6",
+                "scrape_interval": "5s",
+                "static_configs": [{"targets": ["localhost:9091"]}],
+            },
+            {
+                "job_name": "job1",
+                "scrape_interval": "5s",
+                "static_configs": [{"targets": ["localhost:9090"]}],
+            },
+        ]
+        self.assertTrue(len(deepdiff.DeepDiff(_dedupe_job_names(jobs), expected)) == 0)
+        # Make sure the function does not modify its argument
+        self.assertEqual(jobs, jobs_original)