feat: metrics #2

MichaelThamm · 2025-02-06T21:25:51Z

This PR will enable the features:

otelcol can scrape and forward to prom via remote write. Example.
otelcol can receive metrics via remote write.
- This is not yet implemented upstream
- Prometheus Remote-Write receiver Alpha open-telemetry/opentelemetry-collector-contrib#37277
- Step 2: Add integration test to confirm remote-write → remote-write.
Step 1: Add integration test to confirm scrape->remote-write with avalanche – otelcol – prometheus.

bundle: kubernetes
applications:
  av:
    charm: avalanche-k8s
    channel: latest/stable
    revision: 42
    base: [email protected]/stable
    resources:
      avalanche-image: 18
    scale: 1
    trust: true
  otel:
    charm: ./[email protected]
    resources:
      opentelemetry-collector-image: ghcr.io/canonical/opentelemetry-collector:dev
    scale: 1
    trust: true
  prom:
    charm: prometheus-k8s
    channel: latest/stable
    revision: 226
    base: [email protected]/stable
    resources:
      prometheus-image: 151
    scale: 1
    trust: true
relations:
- - av:metrics-endpoint
  - otel:metrics-endpoint
- - prom:receive-remote-write
  - otel:send-remote-write

The metrics from avalanche are consumed by otel-collector and remote-written to prometheus

Further tests

Relate otelcol to multiple prometheus units for remote-writing
juju refresh otelcol to check that the alerts are refreshed without re-relation
juju refresh otelcol to check that the existing alert rules are removed and not leftover

MichaelThamm · 2025-02-06T21:27:35Z

@lucabello I branched off of your feature/first-implementation branch so I can pull in your changes later. I noticed there were some changes regarding the config builder that you did not push. I am pinging you to show you my intentions and also as a request for an initial review to agree on direction.

This PR mainly contributes this in charm.py

…to feature/metrics

…orter add methods

…it doesn't exist

…to feature/metrics

charmcraft.yaml

src/charm.py

sed-i · 2025-02-13T22:53:49Z

src/charm.py

+                    "endpoint": self.remote_write.endpoints[0][
+                        "url"
+                    ],  # TODO Fix this for scalability
+                    "tls": {"insecure": True},


How should we plan to support toggling this for when the tls relation is added?

sed-i · 2025-02-13T22:57:12Z

src/charm.py

+            rule_file = Path(self.metrics_rules_paths.dest) / f"juju_{topology_identifier}.rules"
+            rule_file.write_text(yaml.safe_dump(rule))
+            logger.debug(f"updated alert rules file {rule_file.as_posix()}")
+        self.remote_write.reload_alerts()


Everything before this line doesn't really need self and could be a free function. This could also help with not needing to keep remote_write in self.

src/config.py

lucabello · 2025-02-17T11:18:11Z

src/charm.py

@@ -76,6 +105,70 @@ def _pebble_checks(self) -> Dict[str, Any]:
        }
        return checks

+    def _prometheus_scrape(self, config):


I don't think _prometheus_scrape and _prometheus_remote_write should be in charm.py; the whole point of having a config manager is to let it handle config stuff and keep it out of the charm

As a consequence, I would move updating alert rules outside this function, and call it directly in reconcile instead.

I understand config.py is a "generic" config builder, and most of this method is the receiver config, but if it doesn't belong to either maybe we should think about a third place to put this in?

I think we only need to discuss this in a meeting for me to better understand the design

If the method just handled adding to the config then I would agree but it is a bit more involved. I am happy to discuss in a meeting!

charmcraft.yaml

sed-i · 2025-02-20T22:53:16Z

src/charm.py

+            src=charm_root.joinpath(*self._rules_source_path.split("/")),
+            dest=charm_root.joinpath(*self._rules_destination_path.split("/")),


Nit: self._rules_*_path is used only once. Should we put it here as a string literal instead?

sed-i · 2025-02-20T22:56:04Z

src/charm.py

+            self, alert_rules_path=self.metrics_rules_paths.dest
+        )
+
+        self._reconcile()


Would it make sense to instantiate remote_write and metrics_consumer inside the reconciler (and without making them part of self)?

src/charm.py

sed-i · 2025-02-20T23:09:54Z

src/config.py

@@ -8,6 +8,7 @@
 class Ports(Enum):


Is SimpleNamespace a better fit here?

sed-i · 2025-02-20T23:13:47Z

tests/integration/test_metrics.py

+    av_app_name = "avalanche-k8s"
+    otel_app_name = "otel-collector-k8s"
+    prom_app_name = "prometheus-k8s"
+    await ops_test.model.deploy(av_app_name)
+    await ops_test.model.deploy(charm, otel_app_name, resources=_charm_resources())
+    await ops_test.model.deploy(prom_app_name, trust=True)
+    # WHEN they are related to scrape and remote-write
+    await ops_test.model.integrate(
+        f"{av_app_name}:metrics-endpoint", f"{otel_app_name}:metrics-endpoint"
+    )
+    await ops_test.model.integrate(
+        f"{otel_app_name}:send-remote-write", f"{prom_app_name}:receive-remote-write"
+    )


Should we replace this with a literal bundle?

tests/integration/test_metrics.py

MichaelThamm · 2025-02-21T20:42:14Z

Some useful metrics here:

https://github.com/monitoringartist/opentelemetry-collector-monitoring

Co-authored-by: Leon <[email protected]>

lucabello and others added 13 commits February 5, 2025 12:03

feat: first implementation

edaf496

fix tox commands

10e819c

try to write charm skeleton

a1650b2

fix charm and add gitignore

2600ca9

add framework init()

9bf9730

add tests folder

f2f20a6

change layer name

29b50e4

chore: trigger ci

f2dc88f

add empty test

0cd0ea9

add pebble checks

d227462

fix integration tests

966266f

fix integration tests

3a704e4

feat: metrics in charm

a67c54c

github-actions bot added the Libraries: OK label Feb 6, 2025

lucabello and others added 9 commits February 10, 2025 14:02

pr comments and other changes

2079d47

use CRAFT_ARCH_BUILD_FOR

ef05339

extract get_pebble_checks method

646b2e5

improve config manager

c899406

Merge remote-tracking branch 'origin/feature/first-implementation' in…

e5cb835

…to feature/metrics

fix config manager

783cf7a

fix

b6ae816

Merge remote-tracking branch 'origin/feature/first-implementation' in…

806f8ae

…to feature/metrics

fix config builder

85a9133

github-actions bot added Libraries: Out of Sync and removed Libraries: OK labels Feb 10, 2025

MichaelThamm added 4 commits February 10, 2025 11:43

fix pipeline builder method

f75148b

chore: fix thrift_compact endpoint

1867f67

chore: fix thrift_compact endpoint

459e4e2

fix: container rules dir

9d0384b

MichaelThamm and others added 8 commits February 12, 2025 15:46

chore: update pipeline config for receiver, processor, connector, exp…

a3d01a4

…orter add methods

chore: fix _add_to_pipeline to create the pipeline dict key chain if …

1faa8d4

…it doesn't exist

fix: cleanup ConfigManager and add unit tests for it

3b58643

feat: tests and metrics

298cfdb

Merge remote-tracking branch 'origin/feature/first-implementation' in…

106552f

…to feature/metrics

improve pipelines in config

19728f1

improve tests in pair programming session

72fcfc7

feat: working metrics

0ac8303

MichaelThamm marked this pull request as ready for review February 13, 2025 21:34

sed-i changed the base branch from main to feature/first-implementation February 13, 2025 22:31

sed-i requested a review from a team as a code owner February 13, 2025 22:31

sed-i reviewed Feb 13, 2025

View reviewed changes

feat: add integration tests

2b6d8a2

lucabello requested changes Feb 17, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into feature/metrics

87d4c91

MichaelThamm changed the base branch from feature/first-implementation to main February 19, 2025 15:36

MichaelThamm added 5 commits February 19, 2025 14:00

chore: cleanup

9a4bb4e

chore: fix flaky tests

4a3cf89

chore: cleanup charm methods

e601e86

feat: reload config on changes

ddbe657

feat: scalable prom exporter

c82104a

sed-i reviewed Feb 20, 2025

View reviewed changes

MichaelThamm and others added 4 commits February 21, 2025 16:05

Apply suggestions from code review

0420c70

Co-authored-by: Leon <[email protected]>

chore: cleanup alert rules

195732f

fix: update metrics test

b3b021d

fix: metrics query test

581ea98

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: metrics #2

feat: metrics #2

MichaelThamm commented Feb 6, 2025 •

edited

Loading

MichaelThamm commented Feb 6, 2025 •

edited

Loading

sed-i Feb 13, 2025

sed-i Feb 13, 2025

lucabello Feb 17, 2025

lucabello Feb 17, 2025

MichaelThamm Feb 19, 2025

sed-i Feb 20, 2025

sed-i Feb 20, 2025

sed-i Feb 20, 2025

sed-i Feb 20, 2025

MichaelThamm commented Feb 21, 2025

		src=charm_root.joinpath(*self._rules_source_path.split("/")),
		dest=charm_root.joinpath(*self._rules_destination_path.split("/")),

feat: metrics #2

Are you sure you want to change the base?

feat: metrics #2

Conversation

MichaelThamm commented Feb 6, 2025 • edited Loading

Further tests

MichaelThamm commented Feb 6, 2025 • 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

MichaelThamm commented Feb 21, 2025

MichaelThamm commented Feb 6, 2025 •

edited

Loading

MichaelThamm commented Feb 6, 2025 •

edited

Loading