Skip to content

[grpc][v2] Use v2 gRPC factory in storage extension #6969

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
Merged
merged 3 commits into from
Apr 17, 2025
Merged

[grpc][v2] Use v2 gRPC factory in storage extension #6969

merged 3 commits into from
Apr 17, 2025

Conversation

mahadzaryab1
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 commented Apr 2, 2025 •
edited
Loading

Which problem is this PR solving?

Description of the changes

  • This PR updates the jaegerstorage extension to use the new v2 gRPC storage factory
  • 🛑 Breaking Change: jaeger-v2 no longer supports the v1 remote storage API. 🛑

How was this change tested?

  • CI

Checklist

@mahadzaryab1
Use v2 gRPC Factory
cf9ccb2 
Signed-off-by: Mahad Zaryab <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Apr 3, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.00%. Comparing base (6882354) to head (48332a7).
Report is 54 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6969      +/-   ##
==========================================
- Coverage   96.00%   96.00%   -0.01%     
==========================================
  Files         349      349              
  Lines       20678    20677       -1     
==========================================
- Hits        19852    19851       -1     
  Misses        622      622              
  Partials      204      204
Flag Coverage Δ
badger_v1 9.98% <0.00%> (ø)
badger_v2 2.07% <0.00%> (+<0.01%) ⬆️
cassandra-4.x-v1-manual 15.02% <0.00%> (ø)
cassandra-4.x-v2-auto 2.06% <0.00%> (+<0.01%) ⬆️
cassandra-4.x-v2-manual 2.06% <0.00%> (+<0.01%) ⬆️
cassandra-5.x-v1-manual 15.02% <0.00%> (ø)
cassandra-5.x-v2-auto 2.06% <0.00%> (+<0.01%) ⬆️
cassandra-5.x-v2-manual 2.06% <0.00%> (+<0.01%) ⬆️
elasticsearch-6.x-v1 19.88% <0.00%> (ø)
elasticsearch-7.x-v1 19.96% <0.00%> (ø)
elasticsearch-8.x-v1 20.13% <0.00%> (-0.05%) ⬇️
elasticsearch-8.x-v2 2.07% <0.00%> (+<0.01%) ⬆️
grpc_v1 11.53% <0.00%> (ø)
grpc_v2 9.16% <0.00%> (+0.52%) ⬆️
kafka-3.x-v1 10.26% <0.00%> (ø)
kafka-3.x-v2 2.07% <0.00%> (+<0.01%) ⬆️
memory_v2 2.07% <0.00%> (+<0.01%) ⬆️
opensearch-1.x-v1 20.01% <0.00%> (ø)
opensearch-2.x-v1 20.01% <0.00%> (-0.05%) ⬇️
opensearch-2.x-v2 2.07% <0.00%> (+<0.01%) ⬆️
tailsampling-processor 0.56% <0.00%> (+<0.01%) ⬆️
unittests 94.77% <100.00%> (-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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

yurishkuro
yurishkuro reviewed Apr 3, 2025
View reviewed changes
cmd/jaeger/internal/extension/jaegerstorage/extension.go Outdated
@@ -176,7 +176,7 @@ func (s *storageExt) Start(_ context.Context, host component.Host) error {
grpcTelset := telset
grpcTelset.Metrics = scopedMetricsFactory(storageName, "grpc", "tracestore")
//nolint: contextcheck
Copy link
Member

Choose a reason for hiding this comment

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

How does OTEL Exporter (similar principle of using gRPC client) deal with context in this scenario? At minimum I think we should pass it to the constructor, but more specifically my question is: should the constructor do something with it, like pass it when creating a connection?

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 I updated this code to propagate the context down to the factory. For your question about the exporter, I see this in the exporterhelper - looks like it also accepts the context in the constructor. Is that what you were looking for?

Copy link
Member

Choose a reason for hiding this comment

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

the change lgtm

yurishkuro
yurishkuro previously approved these changes Apr 3, 2025
View reviewed changes
@yurishkuro yurishkuro dismissed their stale review April 3, 2025 02:07

premature approval - this needs remote storage for e2e tests to pass

@mahadzaryab1
Propagate Context
e355f39 
Signed-off-by: Mahad Zaryab <[email protected]>
@yurishkuro yurishkuro added the changelog:exprimental Change to an experimental part of the code label Apr 3, 2025
@mahadzaryab1 mahadzaryab1 marked this pull request as ready for review April 17, 2025 01:16
@mahadzaryab1 mahadzaryab1 requested a review from a team as a code owner April 17, 2025 01:16
@mahadzaryab1 mahadzaryab1 requested a review from yurishkuro April 17, 2025 01:42
@mahadzaryab1 mahadzaryab1 enabled auto-merge April 17, 2025 01:47
yurishkuro
yurishkuro approved these changes Apr 17, 2025
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.

So to confirm - this means v2 write path will be used calling into v2 grpc writer that will use v2 remote storage api? I guess this is where the previous PR is helpful, by allowing remote-storage to support b2 api.

@mahadzaryab1 mahadzaryab1 added this pull request to the merge queue Apr 17, 2025
Merged via the queue into jaegertracing:main with commit 40e1ff3 Apr 17, 2025
59 checks passed
@mahadzaryab1
Copy link
Collaborator Author

So to confirm - this means v2 write path will be used calling into v2 grpc writer that will use v2 remote storage api? I guess this is where the previous PR is helpful, by allowing remote-storage to support b2 api.

@yurishkuro Why would it just be the write path? The extension now initializes the v2 gRPC factory so both the read and write path should be going through the v2 remote storage API. Although, the backing storages will be v1 factories upgraded to v2. Let me know if I'm misunderstanding something though.

@mahadzaryab1 mahadzaryab1 deleted the use-grpc-v2-factory branch April 17, 2025 02:17
amilbcahat pushed a commit to amilbcahat/jaeger that referenced this pull request May 4, 2025
@mahadzaryab1 @amol-verma-allen
[grpc][v2] Use v2 gRPC factory in storage extension (jaegertracing#6969)
fe2dcdc 
## Which problem is this PR solving?
- Resolves jaegertracing#6965

## Description of the changes
- This PR updates the jaegerstorage extension to use the new v2 gRPC
storage factory

## How was this change tested?
- CI

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: Mahad Zaryab <[email protected]>
amilbcahat pushed a commit to amilbcahat/jaeger that referenced this pull request May 4, 2025
@mahadzaryab1 @amol-verma-allen
[grpc][v2] Use v2 gRPC factory in storage extension (jaegertracing#6969)
7665a92 
## Which problem is this PR solving?
- Resolves jaegertracing#6965

## Description of the changes
- This PR updates the jaegerstorage extension to use the new v2 gRPC
storage factory

## How was this change tested?
- CI

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: amol-verma-allen <[email protected]>
@mahadzaryab1 mahadzaryab1 added changelog:breaking-change Change that is breaking public APIs or established behavior and removed changelog:exprimental Change to an experimental part of the code labels May 15, 2025
@denysvitali denysvitali mentioned this pull request May 24, 2025
Open
3 tasks
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

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

Assignees
No one assigned
Labels
area/storage changelog:breaking-change Change that is breaking public APIs or established behavior v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement v2 Factory For Remote Storage
2 participants
@mahadzaryab1 @yurishkuro