8000 feat(tracing): Add proxy tracing configuration to control plane helm chart by sfleen · Pull Request #13994 · linkerd/linkerd2 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(tracing): Add proxy tracing configuration to control plane helm chart #13994

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

Merged
merged 1 commit into from
Jun 26, 2025

Conversation

sfleen
Copy link
Contributor
@sfleen sfleen commented May 5, 2025

Previously, this would require the linkerd-jaeger extension to be installed. This comes with its own set of issues, namely managing yet another component of linkerd.

Since the tracing config is basically just environment variables and one volume mount, hoisting them up into the main control plane helm chart for the proxy injector to handle is likely the simplest and most maintainable path going forward.

The injector from the linkerd-jaeger extension will still work for the time being, and is interoprable to some degree as long as the injector in the extension is disabled. A follow-up to this PR should add documentation around this and how users can migrate from the extension to these configs.

@sfleen sfleen requested a review from a team as a code owner May 5, 2025 21:25
@sfleen sfleen requested review from alpeb and adleong May 5, 2025 21:26
@alpeb
Copy link
Member
alpeb commented May 6, 2025

The injector from the linkerd-jaeger extension will still work for the time being, and is interoprable to some degree as long as the injector in the extension is disabled. A follow-up to this PR should add documentation around this and how users can migrate from the extension to these configs.

IIUC, this should be followed up as well by parametrizing the deployment of the injector in linkerd-jaeger, disabling it by default?

@sfleen sfleen removed the request for review from adleong May 6, 2025 15:29
@sfleen sfleen self-assigned this May 6, 2025
@sfleen sfleen force-pushed the sfleen/proxy-tracing branch from 62c78bc to 812c616 Compare May 8, 2025 14:12
@sfleen sfleen force-pushed the sfleen/proxy-tracing branch 5 times, most recently from 87a4795 to d28fd75 Compare May 28, 2025 13:10
@sfleen sfleen force-pushed the sfleen/proxy-tracing branch from d28fd75 to aa54bd9 Compare June 4, 2025 11:18
Copy link
Member
@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

There still are issues with the whitespace suppression; when enabling tracing I see:

      - emptyDir:
          medium: Memory
        name: linkerd-identity-end-entity- name: linkerd-podinfo
        downwardAPI:
          items:
            - path: labels
              fieldRef:
                fieldPath: metadata.labels

@sfleen sfleen force-pushed the sfleen/proxy-tracing branch from aa54bd9 to 4364e08 Compare June 4, 2025 16:53
@sfleen
Copy link
Contributor Author
sfleen commented Jun 4, 2025

should be fixed, there was some extra whitespace trimming that the templates were doing

@sfleen sfleen assigned alpeb and unassigned sfleen Jun 4, 2025
@sfleen sfleen force-pushed the sfleen/proxy-tracing branch 2 times, most recently from a7390fc to 238b25a Compare June 5, 2025 14:54
Copy link
Member
@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Setting tracing.enable: true while leaving endpoint empty, produces the following error:

$ k -n linkerd logs -f linkerd-identity-6b66cd9c8b-c4bt7 linkerd-proxy
time="2025-06-05T15:11:38Z" level=info msg="Found pre-existing key: /var/run/linkerd/identity/end-entity/key.p8"
time="2025-06-05T15:11:38Z" level=info msg="Found pre-existing CSR: /var/run/linkerd/identity/end-entity/csr.der"
[     0.001322s]  INFO ThreadId(01) linkerd2_proxy: release 2.300.0 (67dc85a) by linkerd on 2025-06-02T22:14:32Z
[     0.002305s] ERROR ThreadId(01) linkerd_app::env::types: Not a valid address:
[     0.002311s] ERROR ThreadId(01) linkerd_app::env: LINKERD2_PROXY_TRACE_COLLECTOR_SVC_ADDR="" is not valid: AddrError(MissingPort)

Is this expected? What do you think about adding some validation at the template level, like what we have in _validate.tpl?

@sfleen sfleen force-pushed the sfleen/proxy-tracing branch from 238b25a to b89b429 Compare June 5, 2025 17:38
@sfleen
Copy link
Contributor Author
sfleen commented Jun 5, 2025

That's super handy, I hadn't done much with helm template validation so that'll help prevent the biggest configuration footgun here.

Copy link
Member
@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Thanks @sfleen , LGTM! 👍

…chart

Previously, this would require the `linkerd-jaeger` extension to be installed. This comes with its own set of issues, namely managing yet another component of linkerd.

Since the tracing config is basically just environment variables and one volume mount, hoisting them up into the main control plane helm chart for the proxy injector to handle is likely the simplest and most maintainable path going forward.

The injector from the `linkerd-jaeger` extension will still work for the time being, and is interoprable to some degree as long as the injector in the extension is disabled. A follow-up to this PR should add documentation around this and how users can migrate from the extension to these configs.

Signed-off-by: Scott Fleener <scott@buoyant.io>
@sfleen sfleen force-pushed the sfleen/proxy-tracing branch from b89b429 to f5eca66 Compare June 26, 2025 17:00
@sfleen sfleen enabled auto-merge (squash) June 26, 2025 17:09
@sfleen sfleen merged commit 26203bd into main Jun 26, 2025
126 of 132 checks passed
@sfleen sfleen deleted the sfleen/proxy-tracing branch June 26, 2025 17:50
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.

3 participants
0