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

Grpc plugin archive storage support #2317

Merged
merged 28 commits into from
Sep 26, 2020
Merged

Grpc plugin archive storage support #2317

merged 28 commits into from
Sep 26, 2020

Conversation

m8rge
Copy link
Contributor

@m8rge m8rge commented Jun 29, 2020

Signed-off-by: Andrew Putilov [email protected]

Which problem is this PR solving?

Resolves #2299

Short description of the changes

  • Added ArchiveSpanReader and ArchiveSpanWriter to shared.StoragePlugin interface.
  • Added to proto/storage.proto ArchiveSpanWriterPlugin and ArchiveSpanReaderPlugin services to serve archive storage methods.
  • New rpc ArchiveSupported() bool to return is archive storage supported or not.
  • Made compatible with old plugins without ArchiveSpanReaderPlugin service implemented. Will act same as archive storage not supported by plugin.

@m8rge m8rge force-pushed the grpc-archive-storage branch 5 times, most recently from ecd9b5c to dd374cf Compare June 29, 2020 11:42
@codecov
Copy link

codecov bot commented Jun 29, 2020 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.83838% with 16 lines in your changes missing coverage. Please review.

Project coverage is 95.36%. Comparing base (87799ea) to head (5d81ec4).

Files with missing lines Patch % Lines
plugin/storage/grpc/shared/plugin.go 0.00% 12 Missing ⚠️
plugin/storage/grpc/grpc.go 0.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2317      +/-   ##
==========================================
+ Coverage   95.21%   95.36%   +0.14%     
==========================================
  Files         208      208              
  Lines        9158     9217      +59     
==========================================
+ Hits         8720     8790      +70     
+ Misses        363      351      -12     
- Partials       75       76       +1

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

@m8rge m8rge marked this pull request as ready for review June 29, 2020 19:16
@m8rge m8rge requested a review from a team as a code owner June 29, 2020 19:16
@m8rge m8rge requested a review from pavolloffay June 29, 2020 19:16
@m8rge m8rge marked this pull request as draft June 29, 2020 19:17
@m8rge m8rge marked this pull request as ready for review June 29, 2020 19:59
@m8rge
Copy link
Contributor Author

m8rge commented Jun 29, 2020 •
edited
Loading

codecov/patch fails due /cmd/query/app/static_handler.go file coverage. I didn't modify it.

@yurishkuro
Copy link
Member

@m8rge ack-ing that I saw the PR, will review over the weekend, unless @pavolloffay gets to it first.

yurishkuro
yurishkuro reviewed Jul 6, 2020
View reviewed changes
plugin/storage/grpc/README.md Outdated Show resolved Hide resolved
plugin/storage/grpc/proto/storage.proto Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Jul 20, 2020
View reviewed changes
plugin/storage/grpc/config/config.go Outdated Show resolved Hide resolved
plugin/storage/grpc/factory.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/grpc_client.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/interface.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/extra/capabilities.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/grpc_server.go Outdated Show resolved Hide resolved
@m8rge
Copy link
Contributor Author

m8rge commented Jul 21, 2020

I'll change README.md last.

@m8rge m8rge requested a review from yurishkuro July 28, 2020 15:38
@m8rge
Copy link
Contributor Author

m8rge commented Aug 4, 2020

@yurishkuro, Could you fill your comments?

yurishkuro
yurishkuro reviewed Aug 4, 2020
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.

Sorry for delay, many other things came up.

My main concern with this approach is the introduction of shared.ArchiveReader/ArchiveWriter interfaces that are pretty redundant. In the main storage we did not have that - we had archive factory, but it was returning normal reader/writer interfaces, and the fact that they are for "archive" was apparent from the context.

I am not convinced by https://github.com/jaegertracing/jaeger/pull/2317/files#r457931354 that these interfaces are necessary. The GRPC methods are already different anyway, and there is no strong need to have a single type implement both main and archive storage.

plugin/storage/grpc/README.md Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Aug 10, 2020
View reviewed changes
plugin/storage/grpc/shared/grpc_client.go Outdated Show resolved Hide resolved
examples/memstore-plugin/main.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/grpc_client_test.go Outdated Show resolved Hide resolved
@m8rge m8rge requested a review from yurishkuro August 11, 2020 11:02
yurishkuro
yurishkuro reviewed Aug 31, 2020
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.

Sorry for the delay. Overall looks good to me.

@joe-elliott @albertteoh you might want to review this, it's a good deep dive into storage APIs

plugin/storage/grpc/shared/archive.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/archive.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/archive.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/archive.go Outdated Show resolved Hide resolved
albertteoh
albertteoh reviewed Sep 2, 2020
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

lgtm, just some minor comments.

plugin/storage/grpc/config/config.go Outdated Show resolved Hide resolved
plugin/storage/grpc/config/config.go Outdated Show resolved Hide resolved
plugin/storage/grpc/shared/archive.go Outdated Show resolved Hide resolved
joe-elliott
joe-elliott reviewed Sep 7, 2020
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Some minor comments/thoughts. After this is merged I'm going to look into improving tests per this discussion #2455 (comment)

plugin/storage/grpc/config/config.go Outdated Show resolved Hide resolved
examples/memstore-plugin/main.go Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Contributor

Other than the conflict, what's the status of this PR?

m8rge and others added 21 commits September 23, 2020 10:08
@m8rge
Bubble up error on Capabilities() call
c2a286a 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Add empty_test.go
d60bb79 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Remove ArchiveReader, ArchiveWriter
7017e62 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Pass config.PluginServices to grpc.Serve
aa7bdcf 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Introduce shared.ArchiveReader/ArchiveWriter
037e8e4 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Improve ArchiveReader/ArchiveWriter according PR comments
347f0e7 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Test fixes
8295249 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Validate plugin type
100657e 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Add context to WriteSpan method
a35182d 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Return plugin capabilities according ArchiveImpl property
dff9797 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Apply changes from master
08e9862 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Extract PluginServices to shared package, introduce ClientPluginServi…
3f19fac 
…ces on plugin client side

Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Improve error text
f93bd8f 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Rebase-related updates
9ceb37f 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Rename memoryStore to memoryStorePlugin
16044f0 
Signed-off-by: Andrew Putilov <[email protected]>
@yurishkuro @m8rge
minor clean-up
5162979 
- make internal types private
- add interface validations
- wrap errors with %w

Signed-off-by: Yuri Shkuro <[email protected]>
@m8rge
Return codes.NotFound in grpc plugin on missing trace
37779f7 
Signed-off-by: Andrew Putilov <[email protected]>
@yurishkuro @m8rge
make fmt
b86cd5d 
Signed-off-by: Yuri Shkuro <[email protected]>
@m8rge
Handle spanstore.ErrTraceNotFound in grpcServer.GetArchiveTrace
b418f35 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Undo unnecessary change
239639a 
Signed-off-by: Yuri Shkuro <[email protected]>
@m8rge
Increase factory code coverage
a3111df 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Copy link
Contributor Author

m8rge commented Sep 24, 2020

@yurishkuro how can I trigger codecov? It stuck in "Waiting for status to be reported" status

@m8rge
Trigger codecov again
c7896e9 
Signed-off-by: Andrew Putilov <[email protected]>
@m8rge
Copy link
Contributor Author

m8rge commented Sep 25, 2020

@yurishkuro Codecov now points to plugin/storage/grpc/config/config.go without tests at all. I didn't know how properly test this lines https://github.com/jaegertracing/jaeger/pull/2317/files#annotation_459564037 because it deep into HashiCorp plugin system. Can we just merge this PR?

@yurishkuro yurishkuro merged commit ecc5091 into jaegertracing:master Sep 26, 2020
@yurishkuro
Copy link
Member

🎉 🎉 🎉

Thanks for your patience, @m8rge. Do you mind creating another PR to explain these changes in plugin/storage/grpc/README.md?

@m8rge m8rge mentioned this pull request Sep 29, 2020
Merged
@m8rge m8rge deleted the grpc-archive-storage branch September 30, 2020 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holycheater holycheater holycheater left review comments

@albertteoh albertteoh albertteoh left review comments

@yurishkuro yurishkuro yurishkuro approved these changes

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@joe-elliott joe-elliott Awaiting requested review from joe-elliott

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gRPC plugin archive storage support
6 participants
@m8rge @yurishkuro @jpkrohling @holycheater @joe-elliott @albertteoh