gRPC Plugin framework #1461

chvck · 2019-04-05T14:06:22Z

Which problem is this PR solving?

Issue #422 POC

Short description of the changes

Plugin based storage backend
This is an extension of the work in #1214 which appears to have become stale.

Still a work in progress. Related to issue jaegertracing#422. Leverages Hashicorp's go-plugin. New set of protos for the use of plugin maintainers. Missing tests. Still need to create a plugin binary for the memory backend. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added Memory GRPC Plugin. Renamed packages (jaegertracing#422) Renamed packages from "external" to "grpc". Changed storage type to "grpc-plugin". Added cmd/memory-grpc-plugin that uses the memory backend as a grpc plugin. Added go-plugin dependency to Glide. Added task to generate plugin protos to Makefile. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Refactored gRPC storage plugin protos Now makes use of protos from model/proto/model.proto and removed mapping functions. Plugin invoker calls the plugin binary directly instead of relying on sh. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Refactored gRPC plugin protos to use defined models Removed mapping code. Removed used of sh, calling plugin binary directly. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Removed .editorconfig file Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added support for gRPC plugin configuration Configuration file is passed down to the plugin through the --config argument. It is the plugin's responsability to parse the file and initialize itself. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added benchmark for noop SpanWriter vs gRPC based noop plugin Fixed issues related to proto changes. Deleted memory-grpc-plugin since it was no longer relevant. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Updated gRPC plugin & protos to implement latest SpanReader Added dependencies to Gopkg.lock since Glide -> dep transition happened recently. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Fixed proto generation introduced when switching to Dep Changed json tags to match when TraceID was not a proto. Signed-off-by: Olivier Boucher <info@olivierboucher.com> proto fix Fixed issue introduced moving TraceID to model.proto Added proper annotations to storage.proto instead of exposing TraceID in model.proto. Tests are back to green. Signed-off-by: Olivier Boucher <info@olivierboucher.com> Added tests for gRPC storage factory and options Moved DependencyLink to model.proto since it had to be used and the existing struct did not implement any Marshaler/Unmarshaler methods. Changed storage configuration to allow testing via mocks. Added DependencyReader interface to the StoragePlugin interface. Signed-off-by: Olivier Boucher <info@olivierboucher.com>

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck · 2019-04-05T14:07:51Z

I've taken the work performed by Olivier, squashed his commits and then rebased onto current master. Fixed up the code to work with the new plugin protobuf model.

yurishkuro

make sure to run make fmt lint. The lint step, especially, fails with many issues.

yurishkuro · 2019-04-06T13:07:34Z

pkg/grpc/config/config.go

+		VersionedPlugins: map[int]plugin.PluginSet{
+			1: shared.PluginMap,
+		},
+		Cmd:              exec.Command(c.PluginBinary, "--config", c.PluginConfigurationFile),


pull up to a variable and disable gosec rule:

// #nosec G204 cmd := exec.Command(c.PluginBinary, "--config", c.PluginConfigurationFile)

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck · 2019-04-16T15:19:43Z

I'm not sure what the best way to add this stuff to the integration tests would be. Possibly to just let it be used with whatever third party grpc plugin is chosen to run? I also see that crossdock is broken, I'm not sure how to go about fixing that.

yurishkuro

there are a lot of conflicts with master, could you try resolving them?

yurishkuro · 2019-04-23T22:50:23Z

pkg/grpc/config/config.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config


this import would read pkg/grpc/config, which doesn't look right considering that it's specific to storage. Any reason not to have it under plugin/storage/grpc?

I had just left it as it was when Olivier wrote it, now moved.

yurishkuro · 2019-04-23T22:54:46Z

the crossdock failure is probably unrelated, but we need to get up to date with master where there may be fixes for it.

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck · 2019-04-25T08:05:31Z

I saw that you pushed a change to the other gRPC plugin framework PR with empty test files, do you want me to pull that over to this too?

yurishkuro · 2019-04-25T23:07:53Z

Oh it's probably because I have the old remote in my local repo with the same branch name.

Yes, I wanted to get the tests and coverage running.

Signed-off-by: Yuri Shkuro <ys@uber.com>

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck · 2019-04-30T16:36:27Z

plugin/storage/grpc/shared/grpc_client.go

+	depsReaderClient storage_v1.DependenciesReaderPluginClient
+}
+
+// DependencyReader implements shared.StoragePlugin.


If GRPCClient doesn't implement StoragePlugin then go-plugin fails to type assert the plugin on creation.

Signed-off-by: Charles Dixon <chvckd@gmail.com>

yurishkuro · 2019-05-01T02:11:44Z

examples/memory-store/main.go

@@ -0,0 +1,65 @@
+// Copyright (c) 2018 The Jaeger Authors.


I think it would be more accurate to call this dir memstore-plugin, since it's an example of storage plugin, not the memory store

yurishkuro · 2019-05-01T02:16:41Z

plugin/storage/grpc/factory_test.go

+}
+
+func (mp *mockPlugin) SpanReader() spanstore.Reader {
+	return mp


small clean-up: can return new mock storage types (e.g. from storage/spanstore/mocks) and remove the actual storage methods with panics below.

yurishkuro · 2019-05-01T02:28:33Z

plugin/storage/grpc/grpc.go

+)
+
+func Serve(implementation shared.StoragePlugin) {
+	plugin.Serve(&plugin.ServeConfig{


can this simply delegate to ServeWithGRPCServer ?

yurishkuro · 2019-05-01T02:37:11Z

plugin/storage/grpc/shared/grpc_server.go

+const spanBatchSize = 1000
+
+// GRPCServer implements shared.StoragePlugin and reads/writes spans and dependencies
+type GRPCServer struct {


does this type need to be public? Looks like it's only used by StorageGRPCPlugin

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck · 2019-05-01T09:15:00Z

plugin/storage/integration/grpc_test.go

+func (s *GRPCStorageIntegrationTestSuite) initialize() error {
+	s.logger, _ = testutils.NewLogger()
+	gopath := os.Getenv("GOPATH")
+	path := gopath + "/src/github.com/jaegertracing/jaeger/examples/memstore-plugin/memstore-plugin"


This hardcoded path feels flakey and requires that the memstore-plugin is already built. I don't really see any better way of doing it though.

plugin/storage/grpc/shared/grpc_client.go

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck · 2019-05-05T10:27:34Z

I'm unsure if I've done something to upset the crossdock part of ci or if something is broken ci side.

yurishkuro · 2019-05-05T15:40:29Z

it keeps getting stuck on installing elasticcurator, which isn't even necessary for crossdock tests. I tried locally, it worked fine, let me restart again.

yurishkuro

👏 👏 👏 🎉 🎉 🎉 🎈 🎈 🎈

yurishkuro · 2019-05-05T18:52:03Z

examples/memstore-plugin/empty_test.go

@@ -0,0 +1,15 @@
+// Copyright (c) 2018 The Jaeger Authors.


this file should be removed (packages with main() are excluded from coverage automatically)

yurishkuro · 2019-05-05T18:53:51Z

plugin/storage/grpc/config/config.go

+}
+
+// Build instantiates a StoragePlugin
+func (c *Configuration) Build() (shared.StoragePlugin, error) {


this file comes up with large gap in coverage, but we can actually test it using examples/memstore-plugin/

yurishkuro · 2019-05-05T19:09:59Z

plugin/storage/grpc/shared/interface.go

+}
+
+// GRPCServer is used by go-plugin to create a grpc plugin server
+func (p *StorageGRPCPlugin) GRPCServer(broker *plugin.GRPCBroker, s *grpc.Server) error {


We could probably add simple tests exercising GRPCServer/GRPCClient methods. I was actually thinking that it would be great to have tests that actually spin up the server and use the client to talk to it. I suppose this is what the integration test is doing, so we should find a way to use it to enhance coverage.

Note to self:

STORAGE=grpc-plugin go test -v -coverpkg ./plugin/storage/grpc/shared -coverprofile coverage.out ./plugin/storage/integration

yurishkuro · 2019-05-05T19:12:36Z

plugin/storage/integration/grpc_test.go

+func (s *GRPCStorageIntegrationTestSuite) initialize() error {
+	s.logger, _ = testutils.NewLogger()
+	gopath := os.Getenv("GOPATH")
+	path := gopath + "/src/github.com/jaegertracing/jaeger/examples/memstore-plugin/memstore-plugin"


tiffon · 2019-05-05T21:00:23Z

W00t! 🎉

olivierboucher and others added 3 commits April 5, 2019 13:34

Remove old generated storage file

1a21e65

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Update plugin grpc interface/implementation

70f43dc

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners April 5, 2019 14:06

chvck mentioned this pull request Apr 5, 2019

WIP - gRPC Plugin framework #1214

Closed

yurishkuro requested changes Apr 6, 2019

View reviewed changes

chvck added 9 commits April 8, 2019 12:18

Fix gosec by adding an exclude comment

482edd6

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Run make fmt

be72ed7

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Remove unused plugin file

9925b38

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Add comments to uncommented functions

74aaf5e

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Split grpc server and client into separate files

a8a0114

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Use errors.wrap rather than fmt for errors

ef0078f

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Add copyright header to grpc server file

931dfa4

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Update headers to updated license on uncommitted files

d5fe102

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Merge branch 'master' into chvck/plugin-framework

f56cc80

yurishkuro reviewed Apr 23, 2019

View reviewed changes

chvck added 2 commits April 24, 2019 09:21

Merge branch 'master' into chvck/plugin-framework

7dc2b9a

Move grpc config to plugin/storage

f180890

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Add empty test files to fix build

419f4ce

Signed-off-by: Yuri Shkuro <ys@uber.com>

chvck added 3 commits April 30, 2019 14:44

Merge branch 'master' into chvck/plugin-framework

dd199ad

Small grpc client refactor

79e0849

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Update grpc interface and move noop plugin to examples

b747fb9

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck commented Apr 30, 2019

View reviewed changes

Add missed fix for factory

03db22b

Signed-off-by: Charles Dixon <chvckd@gmail.com>

yurishkuro reviewed May 1, 2019

View reviewed changes

chvck added 5 commits May 1, 2019 09:24

Rename memory-store example to memstore-plugin

ba8b35f

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Refactor Serve to call ServeWithGRPCServer

616f1b9

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Refactor to make some structs private

bf0e140

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Change factory_test to use mocks

deba568

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Add storage grpc integration test

763ec4c

Signed-off-by: Charles Dixon <chvckd@gmail.com>

chvck commented May 1, 2019

View reviewed changes

chvck commented May 3, 2019

View reviewed changes

plugin/storage/grpc/shared/grpc_client.go Show resolved Hide resolved

chvck added 5 commits May 3, 2019 17:23

Create mocks for storage_v1 and add grpc client tests

3b5e9ea

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Add grpc server tests

6041195

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Run make fmt

86d8a1f

Signed-off-by: Charles Dixon <chvckd@gmail.com>

Merge branch 'master' into chvck/plugin-framework

84a7a67

Strip unnecessary code from test

4a43047

Signed-off-by: Charles Dixon <chvckd@gmail.com>

yurishkuro mentioned this pull request May 5, 2019

Remove ES dependency from crossdock #1514

Merged

Merge branch 'master' into plugin-framework

83f7edc

Main project automation moved this from In progress to Reviewer approved May 5, 2019

yurishkuro approved these changes May 5, 2019

View reviewed changes

yurishkuro changed the title ~~WIP - gRPC Plugin framework~~ gRPC Plugin framework May 5, 2019

yurishkuro merged commit 8588c42 into jaegertracing:master May 5, 2019

Main project automation moved this from Reviewer approved to Done May 5, 2019

yurishkuro mentioned this pull request May 5, 2019

How to support plugins #422

Closed

8 tasks

jpkrohling mentioned this pull request Jul 18, 2019

Comsos DB as storage backend #1667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC Plugin framework #1461

gRPC Plugin framework #1461

chvck commented Apr 5, 2019

chvck commented Apr 5, 2019

yurishkuro left a comment

yurishkuro Apr 6, 2019

chvck commented Apr 16, 2019

yurishkuro left a comment

yurishkuro Apr 23, 2019

chvck Apr 24, 2019

yurishkuro commented Apr 23, 2019

chvck commented Apr 25, 2019

yurishkuro commented Apr 25, 2019

chvck Apr 30, 2019

yurishkuro May 1, 2019

yurishkuro May 1, 2019

yurishkuro May 1, 2019

yurishkuro May 1, 2019

chvck May 1, 2019

yurishkuro May 5, 2019

chvck commented May 5, 2019

yurishkuro commented May 5, 2019

yurishkuro left a comment

yurishkuro May 5, 2019

yurishkuro May 5, 2019

yurishkuro May 5, 2019

yurishkuro May 5, 2019

tiffon commented May 5, 2019

gRPC Plugin framework #1461

gRPC Plugin framework #1461

Conversation

chvck commented Apr 5, 2019

Which problem is this PR solving?

Short description of the changes

chvck commented Apr 5, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chvck commented Apr 16, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Apr 23, 2019

chvck commented Apr 25, 2019

yurishkuro commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chvck commented May 5, 2019

yurishkuro commented May 5, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon commented May 5, 2019