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

[v2][storage] Implement read path for v2 storage interface #6170

Merged
merged 24 commits into from
Dec 6, 2024
Merged

[v2][storage] Implement read path for v2 storage interface #6170

merged 24 commits into from
Dec 6, 2024

Conversation

mahadzaryab1
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 commented Nov 6, 2024 •
edited
Loading

Which problem is this PR solving?

Description of the changes

  • Implemented the read path for the v2 storage interface. This path currently just wraps a v1 span reader and exposes a static method to access the v1 reader.
  • Change the jaeger query extension to initialize a v2 storage factory and obtain the v1 span reader from it.
  • This will unblock the development of more efficient v2 storage implementations, like ClickHouse.

How was this change tested?

  • Added unit tests for new functionality

Checklist

@codecov Codecov
Copy link

codecov bot commented Nov 7, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 96.22%. Comparing base (c367225) to head (6828667).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...md/jaeger/internal/extension/jaegerquery/server.go 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6170      +/-   ##
==========================================
- Coverage   96.24%   96.22%   -0.03%     
==========================================
  Files         356      356              
  Lines       20432    20454      +22     
==========================================
+ Hits        19665    19681      +16     
- Misses        581      585       +4     
- Partials      186      188       +2
Flag Coverage Δ
badger_v1 8.83% <0.00%> (-0.07%) ⬇️
badger_v2 1.62% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1-manual 14.73% <0.00%> (-0.11%) ⬇️
cassandra-4.x-v2-auto 1.56% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-manual 1.56% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1-manual 14.73% <0.00%> (-0.11%) ⬇️
cassandra-5.x-v2-auto 1.56% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-manual 1.56% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 18.46% <0.00%> (-0.15%) ⬇️
elasticsearch-7.x-v1 18.55% <0.00%> (-0.13%) ⬇️
elasticsearch-8.x-v1 18.71% <0.00%> (-0.14%) ⬇️
elasticsearch-8.x-v2 1.62% <0.00%> (+<0.01%) ⬆️
grpc_v1 10.29% <0.00%> (-0.08%) ⬇️
grpc_v2 7.85% <0.00%> (-0.01%) ⬇️
kafka-v1 8.52% <0.00%> (-0.07%) ⬇️
kafka-v2 1.62% <0.00%> (-0.01%) ⬇️
memory_v2 1.62% <0.00%> (+<0.01%) ⬆️
opensearch-1.x-v1 18.59% <0.00%> (-0.14%) ⬇️
opensearch-2.x-v1 18.60% <0.00%> (-0.14%) ⬇️
opensearch-2.x-v2 1.62% <0.00%> (?)
tailsampling-processor 0.45% <0.00%> (-0.01%) ⬇️
unittests 95.13% <94.11%> (-0.03%) ⬇️

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 mahadzaryab1 added changelog:bugfix-or-minor-feature changelog:new-feature Change that should be called out as new feature in CHANGELOG and removed changelog:bugfix-or-minor-feature labels Nov 7, 2024
@mahadzaryab1 mahadzaryab1 marked this pull request as ready for review November 7, 2024 01:22
@mahadzaryab1 mahadzaryab1 requested a review from a team as a code owner November 7, 2024 01:22
@mahadzaryab1 mahadzaryab1 marked this pull request as draft November 10, 2024 20:17
@mahadzaryab1 mahadzaryab1 mentioned this pull request Nov 16, 2024
Merged
4 tasks
@mahadzaryab1 mahadzaryab1 changed the title [WIP][jaeger-v2][storage] Implement read path for v2 storage interface [jaeger-v2][storage] Implement read path for v2 storage interface Nov 16, 2024
@mahadzaryab1 mahadzaryab1 marked this pull request as ready for review November 16, 2024 17:20
@mahadzaryab1 mahadzaryab1 mentioned this pull request Nov 19, 2024
Merged
4 tasks
yurishkuro pushed a commit that referenced this pull request Nov 19, 2024
@mahadzaryab1
[storage][v2] Add reader adapter that just exposes the underlying v1 …
41948cb 
…reader (#6221)

## Which problem is this PR solving?
- Towards #5079 

## Description of the changes
- This PR implements the v2 `spanstore.Reader` interface in the factory
adapter through the `TraceReader`, which is currently just a wrapper
around the v1 `spanstore.Reader`. The `TraceReader` provides a function
`GetV1Reader` that exposes the underlying v1 reader which will be used
in #6170.

## How was this change tested?
- Added unit tests for new functions

## 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`: `yarn lint` and `yarn test`

---------

Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1 mahadzaryab1 changed the title [jaeger-v2][storage] Implement read path for v2 storage interface [v2][storage] Implement read path for v2 storage interface Nov 30, 2024
yurishkuro
yurishkuro reviewed Dec 1, 2024
View reviewed changes
cmd/jaeger/internal/extension/jaegerquery/server.go
}

depReader, err := f.CreateDependencyReader()
depReader, err := v1Factory.CreateDependencyReader()
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to create storage_v2/depstore/ before continuing, so that we don't have to keep this bifurcation?

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 Sure. What do we want to put in that package?

yurishkuro
yurishkuro reviewed Dec 1, 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.

a refactoring suggestion:

  • storage_v/spanstore -> storage_v/tracestore

mahadzaryab1 and others added 2 commits December 2, 2024 10:01
@mahadzaryab1
Merge branch 'main' into v2-reader
4b712d1 
Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1
Remove Unused Imports
d76eeba 
Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1 mahadzaryab1 mentioned this pull request Dec 2, 2024
Merged
4 tasks
@mahadzaryab1
Copy link
Collaborator Author

a refactoring suggestion:

  • storage_v/spanstore -> storage_v/tracestore

#6292

@mahadzaryab1 mahadzaryab1 mentioned this pull request Dec 2, 2024
Merged
4 tasks
yurishkuro
yurishkuro reviewed Dec 3, 2024
View reviewed changes
cmd/query/app/querysvc/query_service.go
@@ -40,15 +42,15 @@ type StorageCapabilities struct {

// QueryService contains span utils required by the query-service.
type QueryService struct {
spanReader spanstore.Reader
traceReader tracestore.Reader
Copy link
Member

Choose a reason for hiding this comment

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

what about archive reader/writer, any reason not to upgrade them too? (or PR is too large?)

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 initially thought that we couldn't do this yet because I thought we were using the ArchiveFactory interface here. However, it seems like we just use the normal reader interface on the archive reader as well (see https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/querysvc/query_service.go#L69) so this can be done. I can do it in this PR or a separate one. Let me know if you have a preference.

Copy link
Member

Choose a reason for hiding this comment

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

Because of the other question (not having v2 reader fully implemented) it's ok to do these separately, no particular benefit afaict.

@yurishkuro
Apply suggestions from code review
9b181e4 
comments / errors

Signed-off-by: Yuri Shkuro <[email protected]>
yurishkuro and others added 3 commits December 5, 2024 10:19
@yurishkuro yurishkuro merged commit 4f45182 into jaegertracing:main Dec 6, 2024
53 of 54 checks passed
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

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

Assignees
No one assigned
Labels
area/storage changelog:new-feature Change that should be called out as new feature in CHANGELOG v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mahadzaryab1 @yurishkuro