feat: ops[tracing] (with a first-party charm lib) #1612

dimaqq · 2025-03-05T04:46:48Z

The tracing facility for the Ops framework.

Quick start.

Declare the charm tracing interface and optionally the TLS interface in your
charmcraft.yaml.

    requires:
        charm-tracing:
            interface: tracing
            limit: 1
            optional: true
        send-ca-cert:
            interface: certificate_transfer
            limit: 1
            optional: true

If you're migrating from the charm-tracing charm lib, you most likely already
have these relations, but do note their names.

Caveat: this library pulls in pydantic, and the Rust build packages must be
specified in your charmcraft.yaml.

    parts:
        charm:
            plugin: charm
            source: .
            build-packages:
                - cargo

If you're migrating from the `charm-tracing` charm lib, that should already be the case.

Then add the Tracing object in your charm::

```py
    import ops

    class Charm(ops.CharmBase):
        def __init__(self, framework: ops.Framework):
        super().__init__(framework)
        ...
        self.tracing = ops.tracing.Tracing(
            self,
            tracing_relation_name='charm-tracing',
            ca_relation_name='send-ca-cert',
        )

Note that you don't have to import ops.tracing or import ops_tracing.
When ops[tracing] has been added to your charm's dependencies, the Ops
library imports this library and re-exports it as ops.tracing.

ops/_tracing/vendor/charms/certificate_transfer_interface/v1/certificate_transfer.py

tonyandrewmeyer

I've mostly looked at _tracing.api since a lot of the rest is the same as in the other PR (I think).

I definitely prefer this approach over the new lifecycle/ops event one. It feels much more natural to only adjust the settings when they change. I think it will be easier to deprecate this when Juju eventually provides most of what's need than in the other approach too. However, I didn't hate the new event approach, so if you feel strongly that it's better then please do make the case for it.

pyproject.toml

ops/_tracing/__init__.py

ops/_tracing/api.py

tonyandrewmeyer · 2025-03-07T04:46:44Z

ops/_tracing/api.py

+        Initialise the tracing thing (FIXME) with the names of these relations.
+
+        ```py
+        import ops.tracing


Does this actually work at the moment? I can't find any import in ops/__init__.py and there doesn't seem to be an ops/tracing.py that imports things from ops/_tracing.py. Is there some magic I'm missing?

If we're only exposing a Tracing and tracer name, then I think we would want to (conditionally) have those available in the ops namespace, so ops.tracer and ops.Tracing rather than ops.tracing.tracer and ops.tracing.Tracing. With Scenario, we already had ops.testing as a namespace, and there are a lot of names there, only needed for testing.

I've written that with the expectation of the monorepo-style refactor, where:

a separate package holds the implementation

ops.tracing re-exports the API

P.S. tracer is or should be private, it's the ops tracer, not the charm tracer.
(akin to logger).

A "tracer" is an object with attributes like name (ops) and version, thus typically each large library has its own. There are no fixed rules there; my 2c is for coarse tracers, e.g.:

ops

charm

[for example] traefik charm lib

ops/_tracing/__init__.py

tonyandrewmeyer

Only an initial review, sorry. I have ignored the vendored files, and haven't had time to review the tests. I also haven't had time to try it out myself yet. I'll try to get to the tests and trying it out early next week.

A bunch of mostly small comments as a starter.

Really nice work! This is clearly very large (even ignoring whitespace changes) and a significant addition to ops and charming. I really like the design and how you've implemented it (other than a few small quibbles mostly around names and privacy).

tonyandrewmeyer · 2025-03-14T09:59:49Z

tracing/readme.md

@@ -0,0 +1,45 @@
+# ops-tracing


Could we name this README.md for consistency?

tonyandrewmeyer · 2025-03-14T10:00:32Z

tracing/readme.md

+# ops-tracing
+
+**First-party OpenTelemetry integration for the [ops](https://pypi.org/project/ops/) framework.**
+This package adds tracing capabilities to ops, enabling you to observe the performance of your applications.


Nit: I believe we're going with "Ops" as the name.

Suggested change

This package adds tracing capabilities to ops, enabling you to observe the performance of your applications.

This package adds tracing capabilities to Ops, enabling you to observe the performance of your applications.

(Elsewhere in the readme as well).

tonyandrewmeyer · 2025-03-14T10:02:56Z

tracing/pyproject.toml

+    {name="The Charm Tech team at Canonical Ltd."},
+]
+classifiers = [
+    "Development Status :: 5 - Production/Stable",


Nit: I know people do it, but I personally don't think Production/Stable should be used with a <1.0.0 version number.

We'll bump the version to match ops before release.

tonyandrewmeyer · 2025-03-14T10:03:50Z

tracing/ops_tracing/const.py

+from dataclasses import dataclass
+
+EXPORT_TIMEOUT: int | float = 1  # seconds
+"""How much to give OTLP span exporter has to push traces to the backend."""


Suggested change

"""How much to give OTLP span exporter has to push traces to the backend."""

"""How much time to give OTLP span exporter to push traces to the backend."""

tonyandrewmeyer · 2025-03-14T10:04:40Z

tracing/ops_tracing/const.py

+
+from dataclasses import dataclass
+
+EXPORT_TIMEOUT: int | float = 1  # seconds


Given the all-caps, probably this should be Final[int | float] instead? Same for the ones below.

Also: do you want to have these as public? Is there a reason that the charmer would use them?

A regular charmer probably wouldn't.
If someone has custom tracing setup, e.g. to some SaaS, then they may want longer timeouts and would be using .set_dest... manually instead of the charm lib.

tonyandrewmeyer · 2025-03-14T11:50:59Z

tracing/ops_tracing/export.py

+import time
+import urllib.error
+import urllib.request
+from pathlib import Path


import pathlib :)

tonyandrewmeyer · 2025-03-14T11:51:27Z

tracing/ops_tracing/export.py

+from opentelemetry.sdk.trace import ReadableSpan
+from opentelemetry.sdk.trace.export import SpanExporter, SpanExportResult


I don't think we want to have these names public. What about renaming this module to _export.py?

tonyandrewmeyer · 2025-03-14T11:51:32Z

tracing/ops_tracing/export.py

+from opentelemetry.sdk.trace import ReadableSpan
+from opentelemetry.sdk.trace.export import SpanExporter, SpanExportResult


I don't think we want to have these names public. What about renaming this module to _export.py?

tonyandrewmeyer · 2025-03-14T11:52:48Z

tracing/ops_tracing/export.py

+
+            return SpanExportResult.SUCCESS
+        except Exception:
+            logger.exception('Exporing tracing data')


Suggested change

logger.exception('Exporing tracing data')

logger.exception('Exporting tracing data')

tonyandrewmeyer · 2025-03-14T11:56:27Z

tracing/ops_tracing/__init__.py

@@ -0,0 +1,73 @@
+# Copyright 2025 Canonical Ltd.


I used src-layout for Scenario, as it seemed to be the general recommendation for new Python projects. Should we use that here too? ops isn't, but is the legacy project in the repo.

Or do you think src-layout was a mistake and maybe we should move Scenario to not use it instead?

I don't feel strongly about it...
I went for the concise option, and it just works?
uv init --package ... does generate an src layout, while plain uv init generates a layoutless app.

I'm not sure about consistency for consistency's sake.

dimaqq · 2025-03-14T11:59:49Z

Note: weirdly the built docs show the content from a few commits back. 🤷🏻

tonyandrewmeyer · 2025-03-14T12:02:44Z

Note: weirdly the built docs show the content from a few commits back. 🤷🏻

Your last published version maybe? See my comment buried somewhere in there about the hack to tox.ini I had to use for Scenario.

dimaqq commented Mar 5, 2025

View reviewed changes

ops/_tracing/vendor/charms/certificate_transfer_interface/v1/certificate_transfer.py Outdated Show resolved Hide resolved

dimaqq force-pushed the feat-otel-ops-tracing-lib branch 2 times, most recently from dd17314 to b283701 Compare March 6, 2025 15:23

dimaqq changed the title ~~very dirty drafty draft: ops[tracing] with a charm lib~~ ops[tracing] with a charm lib Mar 7, 2025

dimaqq changed the title ~~ops[tracing] with a charm lib~~ feat: ops[tracing] with a charm lib Mar 7, 2025

dimaqq force-pushed the feat-otel-ops-tracing-lib branch from ee2f38a to fc56e7d Compare March 7, 2025 03:35

tonyandrewmeyer reviewed Mar 7, 2025

View reviewed changes

dimaqq force-pushed the feat-otel-ops-tracing-lib branch from fc56e7d to ec17c85 Compare March 9, 2025 15:02

dimaqq mentioned this pull request Mar 10, 2025

feat: ops[tracing] #1527

Closed

53 tasks

feat: ops[tracing] with first-party charm lib

4cc1209

dimaqq force-pushed the feat-otel-ops-tracing-lib branch from e1d71b1 to 4cc1209 Compare March 13, 2025 08:51

dimaqq added 3 commits March 13, 2025 20:53

remove stale fixme's

d7779c8

yank the mailing list

ccb093a

implement some unit tests

2b099c7

dimaqq mentioned this pull request Mar 14, 2025

ops[tracing] follow up #1633

Open

3 tasks

dimaqq added 4 commits March 14, 2025 13:19

remove many fixmes, move recursion prevention to juju-log

183cd78

formally catch vendored libs' errors

3f59d39

handle the case where relation returns empty ca list

09eb48f

penultimate fixme

c37ed71

dimaqq mentioned this pull request Mar 14, 2025

feat: port the venv hotfix for https://bugs.launchpad.net/juju/+bug/2058335 #1634

Open

move juju workaround to a separate PR at canonical#1634

ae4c464

dimaqq changed the title ~~feat: ops[tracing] with a charm lib~~ feat: ops[tracing] (with a first-party charm lib) Mar 14, 2025

dimaqq requested review from benhoyt and tonyandrewmeyer March 14, 2025 08:46

dimaqq marked this pull request as ready for review March 14, 2025 08:46

dimaqq added 2 commits March 14, 2025 17:49

repin docs-deps

489762f

fix docs build, than you, Tony!

f6219d2

tonyandrewmeyer reviewed Mar 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ops[tracing] (with a first-party charm lib) #1612

feat: ops[tracing] (with a first-party charm lib) #1612

dimaqq commented Mar 5, 2025 •

edited

Loading

tonyandrewmeyer left a comment

tonyandrewmeyer Mar 7, 2025

dimaqq Mar 9, 2025

dimaqq Mar 9, 2025 •

edited

Loading

tonyandrewmeyer left a comment

tonyandrewmeyer Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

dimaqq Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

dimaqq Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

tonyandrewmeyer Mar 14, 2025

dimaqq Mar 14, 2025

dimaqq commented Mar 14, 2025

tonyandrewmeyer commented Mar 14, 2025

	This package adds tracing capabilities to ops, enabling you to observe the performance of your applications.
	This package adds tracing capabilities to Ops, enabling you to observe the performance of your applications.

	"""How much to give OTLP span exporter has to push traces to the backend."""
	"""How much time to give OTLP span exporter to push traces to the backend."""


		from dataclasses import dataclass

		EXPORT_TIMEOUT: int \| float = 1 # seconds

		from opentelemetry.sdk.trace import ReadableSpan
		from opentelemetry.sdk.trace.export import SpanExporter, SpanExportResult

	logger.exception('Exporing tracing data')
	logger.exception('Exporting tracing data')

feat: ops[tracing] (with a first-party charm lib) #1612

Are you sure you want to change the base?

feat: ops[tracing] (with a first-party charm lib) #1612

Conversation

dimaqq commented Mar 5, 2025 • edited Loading

tonyandrewmeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimaqq Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

tonyandrewmeyer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimaqq commented Mar 14, 2025

tonyandrewmeyer commented Mar 14, 2025

dimaqq commented Mar 5, 2025 •

edited

Loading

dimaqq Mar 9, 2025 •

edited

Loading