Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurable tracing service name #12371

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

GrigoriyMikhalkin
Copy link
Contributor

Part of solution for #11157

linkerd2-proxy part: linkerd/linkerd2-proxy#2856

Introduces new annotation config.linkerd.io/tracing-service-name which allows to set custom tracing service name for proxy.

P.S. Another potential solution would be to use template instead of fixed name, which would allow to set annotation once for namespace, for example. But it has it's own drawback, that in this case we would have to use Pod name or something similar in this template. So, i think the solution with fixed service name annotation is the best.

@GrigoriyMikhalkin GrigoriyMikhalkin requested a review from a team as a code owner March 31, 2024 17:50
@olix0r
Copy link
Member

olix0r commented May 2, 2024

Hi @GrigoriyMikhalkin. Thanks for taking the time to submit this and sorry for the delay and getting you a review.

I'm curious about a slightly modified approach that might lessen the change required. Specifically, I don't think we should anything about tracing should be part of the linkerd-control-plane chart if we can avoid it. Tell me if you think this could work:

jaeger/injector/mutator/patch.go includes tracing-specific proxy configuration, so we should aim to instrument this here. I don't think that we need a new helm value to do this... instead we can use the downward API to reference a dedicated annotation that controls this parameter to set an environment variable. (I'd suggest something like jaeger.linkerd.io/proxy-service-name.) Then, the proxy can use that value as override if it is set and not empty...

@olix0r olix0r self-assigned this May 2, 2024
@GrigoriyMikhalkin GrigoriyMikhalkin force-pushed the configurable-tracing-service-name branch from 1806eff to 398e29c Compare May 20, 2024 10:57
@GrigoriyMikhalkin
Copy link
Contributor Author

GrigoriyMikhalkin commented May 20, 2024

Hi @olix0r!

I agree that use of downward API allows more elegant solution. Updated the PR.

One thing to note is, proxy now relies on annotation named jaeger.linkerd.io/proxy-service-name to be consistent with tracing-enabled annotation. Even though users could use exporters other than Jaeger(e.g. Zipkin). Probably it would make sense to rename these annotations at some point.

@GrigoriyMikhalkin GrigoriyMikhalkin force-pushed the configurable-tracing-service-name branch from d13419e to eeca7dd Compare May 21, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants