Add remote gRPC option for storage plugin #3383

cevian · 2021-11-10T13:55:09Z

Which problem is this PR solving?

Resolves #3377
Resolves #1519

Short description of the changes

Adds the option to host a gRPC storage API on a remote endpoint
using regular gRPC. Previously the plugin system only supported
local socket connections through the go-hashicorp plugin system.

Adds the option to host a gRPC storage API on a remote endpoint using regular gRPC. Previously the plugin system only supported local socket connections through the go-hashicorp plugin system. Signed-off-by: Matvey Arye <[email protected]>

cevian · 2021-11-10T13:58:14Z

I couldn't find how the existing plugin was tested end-to-end, so couldn't write better tests. If more tests are needed, please point me in the right direction.

cevian · 2021-11-10T13:59:36Z

Also wasn't sure about the CLI option naming.

yurishkuro

Can we adapt grpc-plugin storage integration test to test via remote grpc as well?

yurishkuro · 2021-11-10T14:22:55Z

plugin/storage/grpc/options.go

+	pluginServer             = "grpc-storage-plugin.server"
+	pluginTLS                = "grpc-storage-plugin.tls"
+	pluginCAFile             = "grpc-storage-plugin.ca-file"
+	pluginServerHostOverride = "grpc-storage-plugin.server-host-override"
+	pluginConnectionTimeout  = "grpc-storage-plugin.connection-timeout"


Perhaps we can just call it grpc-storage, not the plugin

plugin/storage/grpc/config/config.go

yurishkuro · 2021-11-10T14:37:26Z

plugin test is here

jaeger/Makefile

Lines 105 to 107 in 745587a

    
           grpc-storage-integration-test: 
        
           	(cd examples/memstore-plugin/ && go build .) 
        
           	bash -c "set -e; set -o pipefail; $(GOTEST) -tags=grpc_storage_integration $(STORAGE_PKGS) | $(COLORIZE)"

cevian · 2021-11-10T19:34:27Z

@yurishkuro for the integration tests I will need to either:

modify examples/memstore-plugin to be able to accept a gRPC address cli flag to listen on (and then either start the plugin or the grpc server depending on the flag)
or
create a new examples/memstore-remote to only act as a gRPC server (not the plugin)

Option 1 is less code changes but the examples become a bit more muddled. Do you have any preference if we want a separate example for this?

yurishkuro · 2021-11-10T19:42:09Z

I think option 1 is ok, but I was also thinking that the plugin / grpc server implemented in examples/memstore-plugin is actually pretty boiler plate and just invokes shared plugin/storage/grpc.ServeWithGRPCServer. It might be useful to extend that shared functionality to allow a choice between a unix socket server and a an IP socket. This way we could keep the example itself minimal, we'll just need to expose a CLI flag in it specifying in which mode it should start. Thoughts?

cevian · 2021-11-10T20:08:24Z

Hmm I like that idea. Let me try it out.

plugin/storage/integration/grpc_test.go

plugin/storage/grpc/config/config.go

plugin/storage/grpc/options.go

Use the tlscfg package for the TLS configuration and change the cli option prefix from `grpc-storage-plugin` to `grpc-storage`. Signed-off-by: Matvey Arye <[email protected]>

We also created a RegisterServer method on StorageGRPCPlugin to register be able to easily create remote GRPC servers. Signed-off-by: Matvey Arye <[email protected]>

Also ran `make fmt` Signed-off-by: Matvey Arye <[email protected]>

plugin/storage/grpc/config/config.go

plugin/storage/grpc/options.go

yurishkuro · 2021-11-13T18:08:01Z

plugin/storage/grpc/options.go

+	var tlsFlagsConfig = tlsFlagsConfig()
+	tlsFlagsConfig.AddFlags(flagSet)


Suggested change

var tlsFlagsConfig = tlsFlagsConfig()

tlsFlagsConfig.AddFlags(flagSet)

tlsFlagsConfig().AddFlags(flagSet)

plugin/storage/grpc/options.go

yurishkuro · 2021-11-13T18:09:45Z

plugin/storage/grpc/shared/plugin.go

-// GRPCServer implements plugin.GRPCPlugin. It is used by go-plugin to create a grpc plugin server.
-func (p *StorageGRPCPlugin) GRPCServer(broker *plugin.GRPCBroker, s *grpc.Server) error {
+// RegisterServer registers the plugin with the server
+func (p *StorageGRPCPlugin) RegisterServer(s *grpc.Server) error {


Suggested change

func (p *StorageGRPCPlugin) RegisterServer(s *grpc.Server) error {

func (p *StorageGRPCPlugin) RegisterHandlers(s *grpc.Server) error {

yurishkuro · 2021-11-13T18:13:14Z

plugin/storage/integration/grpc_test.go

+	err := queryPlugin.RegisterServer(s.server)
+	if err != nil {


Suggested change

err := queryPlugin.RegisterServer(s.server)

if err != nil {

if err := queryPlugin.RegisterServer(s.server); err != nil {

yurishkuro · 2021-11-13T18:15:57Z

plugin/storage/integration/grpc_test.go

+		err = s.server.Serve(listener)
+		if err != nil {


Suggested change

err = s.server.Serve(listener)

if err != nil {

if err := s.server.Serve(listener); err != nil {

plugin/storage/integration/grpc_test.go

yurishkuro · 2021-11-13T18:21:28Z

plugin/storage/integration/grpc_test.go

+		"--grpc-storage-plugin.binary", binaryPath,
+		"--grpc-storage-plugin.log-level", "debug",
+	}
+	if configPath != "" {


this check seems unnecessary, will it fail if we append --grpc-storage-plugin.configuration-file with empty string value?

codecov · 2021-11-13T18:29:33Z

Codecov Report

Merging #3383 (5c24f6e) into main (d825152) will increase coverage by 0.02%.
The diff coverage is 67.50%.

@@            Coverage Diff             @@
##             main    #3383      +/-   ##
==========================================
+ Coverage   96.49%   96.51%   +0.02%     
==========================================
  Files         261      262       +1     
  Lines       15277    15307      +30     
==========================================
+ Hits        14741    14774      +33     
+ Misses        453      452       -1     
+ Partials       83       81       -2

Impacted Files	Coverage Δ
plugin/storage/grpc/shared/grpc_client.go	`79.06% <0.00%> (-4.37%)`	⬇️
plugin/storage/grpc/shared/plugin.go	`0.00% <0.00%> (ø)`
plugin/storage/grpc/factory.go	`100.00% <100.00%> (ø)`
plugin/storage/grpc/memory/plugin.go	`100.00% <100.00%> (ø)`
plugin/storage/grpc/options.go	`100.00% <100.00%> (ø)`
plugin/storage/badger/spanstore/reader.go	`96.21% <0.00%> (+0.70%)`	⬆️
cmd/query/app/static_handler.go	`97.60% <0.00%> (+1.79%)`	⬆️
cmd/collector/app/server/zipkin.go	`70.73% <0.00%> (+2.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d825152...5c24f6e. Read the comment docs.

Signed-off-by: Matvey Arye <[email protected]>

cevian · 2021-11-15T16:53:43Z

Thanks for the detailed review @yurishkuro. I think I addressed all the issues

yurishkuro

Please add some tests to plugin/storage/grpc/memory

I recommend running make test lint locally before submitting

yurishkuro · 2021-11-17T00:38:48Z

plugin/storage/integration/grpc_test.go

@@ -87,6 +140,31 @@ func (s *GRPCStorageIntegrationTestSuite) cleanUp() error {
 	return s.initialize()
 }

+type memoryStorePlugin struct {


do you need this type in the test, or can you use one from plugin/storage/grpc/memory/plugin.go ?

plugin/storage/grpc/factory.go

yurishkuro · 2021-11-17T00:45:22Z

plugin/storage/grpc/config/config.go

+	if c.PluginBinary == "" {
+		return c.RemoteTLS.Close()
+	}
+	return nil


Suggested change

if c.PluginBinary == "" {

return c.RemoteTLS.Close()

}

return nil

return c.RemoteTLS.Close()

the check is not necessary, TLSConfig.Close() is safe to call

Signed-off-by: Matvey Arye <[email protected]>

yurishkuro

Thanks!

Signed-off-by: Yuri Shkuro <[email protected]>

With upstream support for remote gPRC storage plugins [1], our query proxy is no longer required. Updated documentation to illustrate how to use jaeger standalone. [1]: jaegertracing/jaeger#3383

cevian requested a review from a team as a code owner November 10, 2021 13:55

cevian requested a review from jpkrohling November 10, 2021 13:55

Add remote gRPC option for storage plugin

c86a3de

Adds the option to host a gRPC storage API on a remote endpoint using regular gRPC. Previously the plugin system only supported local socket connections through the go-hashicorp plugin system. Signed-off-by: Matvey Arye <[email protected]>

cevian force-pushed the add_remote_grpc branch from 292d7a4 to c86a3de Compare November 10, 2021 13:56

yurishkuro reviewed Nov 10, 2021

View reviewed changes

cevian commented Nov 12, 2021

View reviewed changes

plugin/storage/integration/grpc_test.go Outdated Show resolved Hide resolved

cevian requested a review from yurishkuro November 12, 2021 22:54

yurishkuro reviewed Nov 13, 2021

View reviewed changes

plugin/storage/grpc/config/config.go Outdated Show resolved Hide resolved

plugin/storage/grpc/options.go Outdated Show resolved Hide resolved

cevian added 3 commits November 13, 2021 13:04

Change remote gRPC configuration parameters

d75927e

Use the tlscfg package for the TLS configuration and change the cli option prefix from `grpc-storage-plugin` to `grpc-storage`. Signed-off-by: Matvey Arye <[email protected]>

Add tests for remote gRPC

3714ebe

We also created a RegisterServer method on StorageGRPCPlugin to register be able to easily create remote GRPC servers. Signed-off-by: Matvey Arye <[email protected]>

Fixing naming brought up in PR review

fb3e2bc

Also ran `make fmt` Signed-off-by: Matvey Arye <[email protected]>

cevian force-pushed the add_remote_grpc branch from 564b78d to fb3e2bc Compare November 13, 2021 18:04

yurishkuro reviewed Nov 13, 2021

View reviewed changes

cevian added 2 commits November 15, 2021 11:50

Fixing code nits for GRPC remote storage

44a7ad6

Signed-off-by: Matvey Arye <[email protected]>

Refactor to combine the memory storage plugin

903fe1a

Signed-off-by: Matvey Arye <[email protected]>

yurishkuro reviewed Nov 17, 2021

View reviewed changes

yurishkuro and others added 3 commits November 16, 2021 19:47

Merge branch 'master' into add_remote_grpc

5d06067

Fix nits in PR review

539dbca

Signed-off-by: Matvey Arye <[email protected]>

Add test for grpc memory plugin

c4a1b01

Signed-off-by: Matvey Arye <[email protected]>

cevian force-pushed the add_remote_grpc branch from 476354f to c4a1b01 Compare December 10, 2021 00:57

yurishkuro previously approved these changes Dec 10, 2021

View reviewed changes

yurishkuro enabled auto-merge (squash) December 10, 2021 14:52

yurishkuro and others added 2 commits December 10, 2021 09:52

Merge branch 'main' into add_remote_grpc

f0e8785

Add test for Close

5c24f6e

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro dismissed their stale review via 5c24f6e December 10, 2021 16:45

yurishkuro disabled auto-merge December 10, 2021 20:30

yurishkuro merged commit 1b74ef9 into jaegertracing:main Dec 10, 2021

yurishkuro mentioned this pull request Dec 10, 2021

Change crossdock end-to-end test to use "fast to start" storage #1221

Closed

JamesGuthrie mentioned this pull request Jan 18, 2022

Remove obsoleted jaeger-query-proxy timescale/promscale#1063

Merged

2 tasks

VineethReddy02 mentioned this pull request Jul 13, 2022

Add Promscale as supported backend for Jaeger jaegertracing/documentation#583

Merged

yurishkuro mentioned this pull request Aug 1, 2022

[Feature]: Shared remote storage via Remote Storage API #3835

Closed

4 tasks

yurishkuro mentioned this pull request Aug 26, 2022

Support gRPC API for storage #1519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remote gRPC option for storage plugin #3383

Add remote gRPC option for storage plugin #3383

cevian commented Nov 10, 2021 •

edited by yurishkuro

Loading

cevian commented Nov 10, 2021

cevian commented Nov 10, 2021

yurishkuro left a comment

yurishkuro Nov 10, 2021

yurishkuro commented Nov 10, 2021

cevian commented Nov 10, 2021

yurishkuro commented Nov 10, 2021

cevian commented Nov 10, 2021

yurishkuro Nov 13, 2021

yurishkuro Nov 13, 2021

yurishkuro Nov 13, 2021

yurishkuro Nov 13, 2021

yurishkuro Nov 13, 2021

codecov bot commented Nov 13, 2021 •

edited

Loading

cevian commented Nov 15, 2021

yurishkuro left a comment

yurishkuro Nov 17, 2021

yurishkuro Nov 17, 2021

yurishkuro left a comment

		var tlsFlagsConfig = tlsFlagsConfig()
		tlsFlagsConfig.AddFlags(flagSet)

	var tlsFlagsConfig = tlsFlagsConfig()
	tlsFlagsConfig.AddFlags(flagSet)
	tlsFlagsConfig().AddFlags(flagSet)

	func (p StorageGRPCPlugin) RegisterServer(s grpc.Server) error {
	func (p StorageGRPCPlugin) RegisterHandlers(s grpc.Server) error {

	err := queryPlugin.RegisterServer(s.server)
	if err != nil {
	if err := queryPlugin.RegisterServer(s.server); err != nil {

	err = s.server.Serve(listener)
	if err != nil {
	if err := s.server.Serve(listener); err != nil {

Add remote gRPC option for storage plugin #3383

Add remote gRPC option for storage plugin #3383

Conversation

cevian commented Nov 10, 2021 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

cevian commented Nov 10, 2021

cevian commented Nov 10, 2021

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Nov 10, 2021

cevian commented Nov 10, 2021

yurishkuro commented Nov 10, 2021

cevian commented Nov 10, 2021

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

codecov bot commented Nov 13, 2021 • edited Loading

Codecov Report

cevian commented Nov 15, 2021

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 left a comment

Choose a reason for hiding this comment

cevian commented Nov 10, 2021 •

edited by yurishkuro

Loading

codecov bot commented Nov 13, 2021 •

edited

Loading