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

[fix][grpc] Disable tracing in grpc storage writer clients #6125

Merged
merged 8 commits into from
Oct 27, 2024
Merged

[fix][grpc] Disable tracing in grpc storage writer clients #6125

merged 8 commits into from
Oct 27, 2024

Conversation

mahadzaryab1
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 commented Oct 26, 2024 •
edited
Loading

Which problem is this PR solving?

Description of the changes

  • This PR fixes an issue where the GRPC remote storage client was provided a tracer which was resulting in an infinite loop of trace generation. This infinite loop would happen when we would try to write a trace to storage which would generate a new trace that needed to be written and so on. This PR provides a fix for this by using a noop tracer for the writer clients so that we do not generate traces on the write paths but still do so when reading.
  • This is likely just a temporary fix and we'll want to monitor Ability to customize internal telemetry open-telemetry/opentelemetry-collector#10663 for a better long-term fix.

How was this change tested?

Checklist

Co-Authors

This PR is a continuation of #5979
Co-authored-by: cx 1249843194@qq.com

@mahadzaryab1
Create Writer Clients With No Tracing
10a71ad 
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1
Copy link
Collaborator Author

@yurishkuro where would be a good place to write a test for this?

@codecov Codecov
Copy link

codecov bot commented Oct 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.41%. Comparing base (85d558d) to head (d2366e4).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
plugin/storage/grpc/factory.go 95.83% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6125      +/-   ##
==========================================
- Coverage   96.42%   96.41%   -0.02%     
==========================================
  Files         353      353              
  Lines       20112    20137      +25     
==========================================
+ Hits        19394    19416      +22     
- Misses        533      535       +2     
- Partials      185      186       +1
Flag Coverage Δ
badger_v1 8.32% <0.00%> (-0.03%) ⬇️
badger_v2 1.68% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1 14.39% <0.00%> (-0.04%) ⬇️
cassandra-4.x-v2 1.62% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1 14.39% <0.00%> (-0.04%) ⬇️
cassandra-5.x-v2 1.62% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 18.51% <0.00%> (-0.04%) ⬇️
elasticsearch-7.x-v1 18.58% <0.00%> (-0.05%) ⬇️
elasticsearch-8.x-v1 18.76% <0.00%> (-0.04%) ⬇️
elasticsearch-8.x-v2 1.68% <0.00%> (+<0.01%) ⬆️
grpc_v1 9.53% <89.28%> (+0.16%) ⬆️
grpc_v2 7.01% <0.00%> (-0.02%) ⬇️
kafka-v1 8.88% <0.00%> (-0.03%) ⬇️
kafka-v2 1.68% <0.00%> (-0.01%) ⬇️
memory_v2 1.68% <0.00%> (+<0.01%) ⬆️
opensearch-1.x-v1 18.64% <0.00%> (-0.05%) ⬇️
opensearch-2.x-v1 18.64% <0.00%> (-0.04%) ⬇️
opensearch-2.x-v2 1.67% <0.00%> (-0.02%) ⬇️
tailsampling-processor 0.47% <0.00%> (-0.01%) ⬇️
unittests 95.33% <96.42%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mahadzaryab1
Use V2 Endpoint in GRPC Storage For Testing [TODO: Revert This Commit]
e8c6937 
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1
Copy link
Collaborator Author

mahadzaryab1 commented Oct 26, 2024 •
edited
Loading

For the time being - I'm using the v2 healthcheck endpoint which wasn't working in #6113. It looks to be working with this change in place now.

@mahadzaryab1
Minimize Code Movements
c21b1e6 
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
yurishkuro
yurishkuro reviewed Oct 26, 2024
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

If this closes any other tickets or prs please include in description.

Also give credit to the original author. Did you reuse any of their commits? When you go GitHub automatically includes them in the PR summary.

plugin/storage/grpc/factory.go Outdated Show resolved Hide resolved
plugin/storage/grpc/factory.go Show resolved Hide resolved
@mahadzaryab1
Address Feedback From PR Review
69824ba 
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1 mahadzaryab1 changed the title [WIP] [fix][grpc] Disable tracing in grpc storage writer clients [fix][grpc] Disable tracing in grpc storage writer clients Oct 26, 2024
@mahadzaryab1
Copy link
Collaborator Author

mahadzaryab1 commented Oct 26, 2024 •
edited
Loading

If this closes any other tickets or prs please include in description.

Done

Also give credit to the original author. Did you reuse any of their commits? When you go GitHub automatically includes them in the PR summary.

I didn't reuse any of their commits since I didn't have permissions to modify the PR so I started from scratch. I manually edited the PR description to add the original author as a co-author for this PR.

@mahadzaryab1
Fix Unit Test
0e0dddb 
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1 mahadzaryab1 marked this pull request as ready for review October 26, 2024 18:01
@mahadzaryab1 mahadzaryab1 requested a review from a team as a code owner October 26, 2024 18:01
@yurishkuro
Copy link
Member

didn't reuse any of their commits since I didn't have permissions to modify the PR

For the future - you don't need to modify their pr, just do 'gh pr checkout ###' then create a new branch and push it to a new pr.

yurishkuro
yurishkuro reviewed Oct 26, 2024
View reviewed changes
plugin/storage/grpc/factory.go Outdated
opts = append(opts, grpc.WithStreamInterceptor(bearertoken.NewStreamClientInterceptor()))

remoteConn, err := newClient(opts...)
baseOpts = append(baseOpts, grpc.WithUnaryInterceptor(bearertoken.NewUnaryClientInterceptor()))
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure declaring two options for an interceptor type won't have latest-wins effect? In the pr that introduced bearer interceptors we used a composite interceptor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yurishkuro looks like grpc.NewClient chains the interceptors into one (ref: https://github.com/grpc/grpc-go/blob/cb329375b14e0ddd4e454e18405a732f7c2ccd72/clientconn.go#L170-L171)

	chainUnaryClientInterceptors(cc)
	chainStreamClientInterceptors(cc)

Copy link
Member

Choose a reason for hiding this comment

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

Not sure about that, but the way the functional options are processed is they just override the only interceptor slot: https://github.com/grpc/grpc-go/blob/cb329375b14e0ddd4e454e18405a732f7c2ccd72/dialoptions.go#L535

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

chained the unary and stream interceptors together

plugin/storage/grpc/factory.go Outdated Show resolved Hide resolved
plugin/storage/grpc/factory.go Show resolved Hide resolved
@mahadzaryab1
Clone Slice Before Appending
b6b1911 
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1
Address Feedback From PR Review
aba573d 
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1
Chain Interceptors
d2366e4 
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@yurishkuro yurishkuro merged commit a4efe57 into jaegertracing:main Oct 27, 2024
49 checks passed
@yurishkuro yurishkuro mentioned this pull request Oct 31, 2024
Closed
yurishkuro added a commit to yurishkuro/jaeger that referenced this pull request Nov 2, 2024
@yurishkuro
Revert "[fix][grpc] Disable tracing in grpc storage writer clients (j…
d7fb346 
…aegertracing#6125)"

This reverts commit a4efe57.
@mahadzaryab1 mahadzaryab1 deleted the fix-grpc-factory branch November 23, 2024 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@pavolloffay pavolloffay Awaiting requested review from pavolloffay pavolloffay is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
area/storage bug changelog:bugfix-or-minor-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[jaeger-v2] Dangerous use of tracer in plugin/storage/grpc/factory.go
2 participants
@mahadzaryab1 @yurishkuro