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

Allow to change OcCollector Service name via LINKERD2_PROXY_TRACING_S… #2856

Conversation

GrigoriyMikhalkin
Copy link

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 67.63%. Comparing base (96124bc) to head (952951a).
Report is 256 commits behind head on main.

Current head 952951a differs from pull request most recent head 3fd2bf1

Please upload reports for the commit 3fd2bf1 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
- Coverage   67.68%   67.63%   -0.05%     
==========================================
  Files         332      333       +1     
  Lines       15158    15182      +24     
==========================================
+ Hits        10259    10269      +10     
- Misses       4899     4913      +14     
Files Coverage Δ
linkerd/app/src/oc_collector.rs 26.08% <ø> (ø)
linkerd/app/src/env.rs 59.15% <50.00%> (-0.24%) ⬇️

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bd02c4...3fd2bf1. Read the comment docs.

@@ -249,6 +249,8 @@ const ENV_OUTBOUND_HTTP1_CONNECTION_POOL_IDLE_TIMEOUT: &str =

const ENV_SHUTDOWN_GRACE_PERIOD: &str = "LINKERD2_PROXY_SHUTDOWN_GRACE_PERIOD";

const ENV_PROXY_TRACING_SERVICE_NAME: &str = "LINKERD2_PROXY_TRACING_SERVICE_NAME";
Copy link
Member

Choose a reason for hiding this comment

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

For the sake of consistency, we already have:

pub const ENV_TRACE_ATTRIBUTES_PATH: &str = "LINKERD2_PROXY_TRACE_ATTRIBUTES_PATH";

can we call this LINKERD2_PROXY_TRACE_SERVICE_NAME and group the definitions?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@GrigoriyMikhalkin GrigoriyMikhalkin force-pushed the configurable-tracing-service-name branch 2 times, most recently from c934687 to 3fd2bf1 Compare May 20, 2024 13:25
@GrigoriyMikhalkin GrigoriyMikhalkin force-pushed the configurable-tracing-service-name branch from 3fd2bf1 to 0ffe5c5 Compare July 28, 2024 18:02
@GrigoriyMikhalkin
Copy link
Author

@alpeb Do i understand it correctly that it was already implemented(by a different approach) in linkerd/linkerd2#13130? So this PR can be closed, i assume?

@sfleen
Copy link
Collaborator

sfleen commented Oct 11, 2024

Correct, we should be able to close this PR now.

@sfleen sfleen closed this Oct 11, 2024
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.

4 participants