This repository has been archived by the owner on Oct 8, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 3
Switch to otlp/http protocol in charm_tracing #55
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
85f6596
to
2c353e3
Compare
PietroPasotti
suggested changes
Jan 2, 2024
There was a problem hiding this 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
2ce2379
to
8a981bb
Compare
601ca3d
to
97c8773
Compare
PietroPasotti
suggested changes
Jan 4, 2024
There was a problem hiding this 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?
sed-i
reviewed
Jan 4, 2024
PietroPasotti
approved these changes
Jan 5, 2024
There was a problem hiding this 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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
withopentelemetry-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 providingcertificate_file
instead of the currentcertificate
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.