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

[v2] Add v1 factory converter to v2 storage factory #5497

Merged
merged 21 commits into from
Jun 19, 2024

Conversation

james-ryans
Copy link
Contributor

@james-ryans james-ryans commented May 28, 2024

Which problem is this PR solving?

Description of the changes

  • Current storageextension uses v1 storage factory and transitioning from v1 to v2 requires v2 to implement the v1 storage factory interface. This PR helps by creating a wrapper factory to implement v1 storage factory converted from v2 spanstore interface.

How was this change tested?

  • unit tests

Checklist

Signed-off-by: James Ryans <james.ryans2012@gmail.com>
@james-ryans james-ryans requested a review from a team as a code owner May 28, 2024 17:14
@yurishkuro yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label May 28, 2024
Copy link

codecov bot commented May 28, 2024

Codecov Report

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

Project coverage is 96.38%. Comparing base (1c1f22b) to head (603bbbc).

Files Patch % Lines
...l/extension/jaegerstorage/factoryadapter/writer.go 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5497   +/-   ##
=======================================
  Coverage   96.37%   96.38%           
=======================================
  Files         327      329    +2     
  Lines       16028    16056   +28     
=======================================
+ Hits        15447    15475   +28     
  Misses        404      404           
  Partials      177      177           
Flag Coverage Δ
badger_v1 8.04% <ø> (ø)
badger_v2 1.92% <0.00%> (-0.01%) ⬇️
cassandra-3.x-v1 16.60% <ø> (ø)
cassandra-3.x-v2 1.84% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1 16.60% <ø> (ø)
cassandra-4.x-v2 1.84% <0.00%> (-0.01%) ⬇️
elasticsearch-7.x-v1 18.87% <ø> (ø)
elasticsearch-8.x-v1 19.06% <ø> (ø)
elasticsearch-8.x-v2 19.06% <ø> (ø)
grpc_v1 9.47% <ø> (ø)
grpc_v2 7.49% <0.00%> (-0.03%) ⬇️
kafka 9.76% <ø> (ø)
opensearch-1.x-v1 18.92% <ø> (ø)
opensearch-2.x-v1 18.92% <ø> (ø)
opensearch-2.x-v2 18.92% <ø> (+0.01%) ⬆️
unittests 94.23% <95.12%> (+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.

Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
@james-ryans james-ryans changed the title [v2] Add factory converter to v1 storage factory [v2] Add v1 factory converter to v2 storage factory Jun 5, 2024
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.

lgtm. The Reader part is not urgent since we don't have places that would need OTLP (the only place is the api_v3 endpoint).

Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
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.

what's your plan for next steps? Add tests, or try to use this for real?

Signed-off-by: James Ryans <james.ryans2012@gmail.com>
@james-ryans
Copy link
Contributor Author

what's your plan for next steps? Add tests, or try to use this for real?

I'm thinking of using this for real directly. Because I think this is a temporary package to support both v1 & v2 while we transitioning the storage plugin implementations to v2. But what do you think about that? Should I add the tests to minimize the error rate?

@yurishkuro
Copy link
Member

I think the best way to validate API is to use it! Right now jaegerexporter operates directly on v1 storage API. What would it take to switch it to operate on v2 API? We can make a change to the exporter AND to the jaegerstorage extension such that the latter would provide a lookup method for getting a V2 factory to the exporter, by wrapping v1 factories internally into this new adapter. This will, incidentally, improve the coverage of your new code since it will be exercised in the e2e tests. You might only need to add manual tests for error conditions.

I would consider perhaps pulling the Reader into a separate PR from this one, since jaeger-v2 won't have an easy way to exercise v2->v1 adapter for the read path.

Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
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.

lgtm, can we add some tests?

@james-ryans
Copy link
Contributor Author

lgtm, can we add some tests?

yes, I'm working on it

Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
Signed-off-by: James Ryans <james.ryans2012@gmail.com>
@yurishkuro yurishkuro merged commit 3f6f5fe into jaegertracing:main Jun 19, 2024
40 checks passed
@yurishkuro
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants