Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Switch to otlp/http protocol in charm_tracing #55

Merged
merged 11 commits into from
Jan 5, 2024
Merged

Conversation

mmkay
Copy link
Contributor

@mmkay mmkay commented Dec 13, 2023

Issue

Inclusion of charm_tracing library caused a dependency on GRPC that, in various contexts, added between 10MB to 100+MB to the charm size using this library (ref: canonical/prometheus-k8s-operator#545 ). We considered other protocols to send traces and decided that OTLP/HTTP seems to be the most relevant in the current context.

Resolves #53.

Solution

Replacement of opentelemetry-exporter-otlp-proto-grpc with opentelemetry-exporter-otlp-proto-http together with bumping up to the latest (1.21.0) version.

The main difference between exporters is that instead of certificate being passed as one of the parameters of OTLPSpanExporter, HTTP exporter asks for a path to a certificate file. That means that the library upgrade for charms that are using a certificate (traefik-k8s-operator) requires changing the annotation signature and providing certificate_file instead of the current certificate parameter.

Context

One interesting side effect of the version upgrade is that _get_tracer() couldn't find the context var for tracer in its context when an external function was instrumented using @trace. There was another context var reference with the same name pointing at the same tracer object in its context, though, so I added a fallback trace retrieval.

Testing Instructions

Integration tests set up tempo together with a tester charm that sends traces and confirms they were ingested correctly.

Release Notes

Replacement of OTLP/GRPC protocol with OTLP/HTTP in traces export.

Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! A couple of concerns/API design questions we can discuss about

lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
@mmkay mmkay force-pushed the switch-to-otlp-http branch from 2ce2379 to 8a981bb Compare January 3, 2024 14:17
@mmkay mmkay requested a review from PietroPasotti January 3, 2024 14:54
@mmkay mmkay force-pushed the switch-to-otlp-http branch from 601ca3d to 97c8773 Compare January 4, 2024 13:07
@mmkay mmkay requested a review from IbraAoad as a code owner January 4, 2024 13:07
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few missing bits, but we're getting there.

I'm assuming you tried it out manually to see if it works?

lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/tracing.py Show resolved Hide resolved
lib/charms/tempo_k8s/v1/tracing.py Outdated Show resolved Hide resolved
@mmkay
Copy link
Contributor Author

mmkay commented Jan 4, 2024

@PietroPasotti

I'm assuming you tried it out manually to see if it works?

Correct - manual reproduction steps, assuming tempo is mounted on the multipass host under ~/tempo-k8s-operator and libraries are copied to tester charm:

juju add-model cos
juju switch cos
juju deploy cos-lite --trust
cd tempo-k8s-operator
charmcraft pack 
charmcraft pack -p ./tests/integration/tester/
juju deploy ./tempo-k8s_ubuntu-22.04-amd64.charm tempo --resource tempo-image=grafana/tempo:1.5.0
juju deploy ./tester_ubuntu-22.04-amd64.charm tester -n 3 --resource workload=python:slim-buster
juju relate tempo:tracing tester:tracing
juju relate grafana:grafana-source tempo:grafana-source

You should be able to see traces from TempoTesterCharm in Grafana:
Screenshot from 2024-01-04 20-57-55

charmcraft.yaml Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
tests/integration/tester/src/charm.py Outdated Show resolved Hide resolved
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice! GJ. No need from another pass from my side.

lib/charms/tempo_k8s/v1/charm_tracing.py Outdated Show resolved Hide resolved
lib/charms/tempo_k8s/v1/charm_tracing.py Show resolved Hide resolved
@mmkay mmkay merged commit 18b600f into main Jan 5, 2024
13 checks passed
@mmkay mmkay deleted the switch-to-otlp-http branch January 5, 2024 16:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider alternatives to GRPC for charm_tracing
4 participants