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.

Sign up for GitHub

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

Jump to bottom

stats/opentelemetry: use TextMapProvider and TracerProvider from TraceOptions instead of otel global #8166

Merged
merged 2 commits into from
Mar 13, 2025
Merged

stats/opentelemetry: use TextMapProvider and TracerProvider from TraceOptions instead of otel global #8166

merged 2 commits into from
Mar 13, 2025

Conversation

vinothkumarr227
Copy link
Contributor

@vinothkumarr227 vinothkumarr227 commented Mar 12, 2025 •
edited by dfawley
Loading

RELEASE NOTES:

  • stats/opentelemetry: fix tracing to use TextMapPropagator and TracerProvider from TraceOptions instead of OpenTelemetry globals.

@codecov Codecov
Copy link

codecov bot commented Mar 12, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.38%. Comparing base (5ac9042) to head (cb90395).
Report is 3 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8166      +/-   ##
==========================================
+ Coverage   82.33%   82.38%   +0.04%     
==========================================
  Files         392      393       +1     
  Lines       39119    39143      +24     
==========================================
+ Hits        32210    32247      +37     
+ Misses       5593     5579      -14     
- Partials     1316     1317       +1
Files with missing lines Coverage Δ
stats/opentelemetry/client_tracing.go 76.47% <100.00%> (ø)
stats/opentelemetry/server_tracing.go 100.00% <100.00%> (ø)

... and 20 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@purnesh42H purnesh42H changed the title Refactor: Use TraceOptions TextMapPropagator stats/opentelemetry: use TextMapProvider and Tracer from TraceOptions instead of otel global Mar 12, 2025
@purnesh42H purnesh42H changed the title stats/opentelemetry: use TextMapProvider and Tracer from TraceOptions instead of otel global stats/opentelemetry: use TextMapProvider and TracerProvider from TraceOptions instead of otel global Mar 12, 2025
@purnesh42H purnesh42H self-requested a review March 12, 2025 18:32
purnesh42H
purnesh42H reviewed Mar 12, 2025
View reviewed changes
Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for quick fix.

stats/opentelemetry/client_tracing.go Outdated
otelinternaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing"
)

const traceName = "grpc-go"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: tracerName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@purnesh42H purnesh42H self-requested a review March 12, 2025 18:35
@purnesh42H purnesh42H added Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability Type: Bug labels Mar 12, 2025
@purnesh42H purnesh42H added this to the 1.72 Release milestone Mar 12, 2025
@purnesh42H
Copy link
Contributor

@dfawley for second review. Not sure why dependency check is failing. Is it because we removed usages of otel?

@purnesh42H purnesh42H mentioned this pull request Mar 12, 2025
Closed
dfawley
dfawley approved these changes Mar 12, 2025
View reviewed changes
stats/opentelemetry/client_tracing.go
otelinternaltracing "google.golang.org/grpc/stats/opentelemetry/internal/tracing"
)

const tracerName = "grpc-go"
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this mentioned in the PR description - why is this changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

to keep same as metrics https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/client_metrics.go#L51

@dfawley dfawley assigned purnesh42H and unassigned dfawley Mar 12, 2025
@purnesh42H purnesh42H merged commit 775150f into grpc:master Mar 13, 2025
14 of 15 checks passed
purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Mar 19, 2025
@vinothkumarr227 @purnesh42H
stats/opentelemetry: use TextMapProvider and TracerProvider from Trac…
daeca53 
…eOptions instead of otel global (grpc#8166)
@purnesh42H purnesh42H mentioned this pull request Mar 19, 2025
Merged
purnesh42H added a commit that referenced this pull request Mar 20, 2025
@purnesh42H
Cherry-pick #8164 #8166 to v1.71.x (#8182)
208e03b
janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Mar 24, 2025
@vinothkumarr227 @janardhankrishna-sai
stats/opentelemetry: use TextMapProvider and TracerProvider from Trac…
b510f30 
…eOptions instead of otel global (grpc#8166)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@purnesh42H purnesh42H purnesh42H approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@purnesh42H purnesh42H

Labels
Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability Type: Bug
Projects
None yet
Milestone
1.72 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@vinothkumarr227 @purnesh42H @dfawley