[jaeger-v2] Add support for GRPC storarge #5228

james-ryans · 2024-02-24T10:50:06Z

Which problem is this PR solving?

Part of Jaeger v2 based on OpenTelemetry collector #4843
Separate GRPC storage PR from and will be used by jaeger-v2 Kafka PR [jaeger-v2] Add kafka exporter and receiver configuration #4971

Description of the changes

Implement GRPC storage backend for Jaeger-V2 storage

How was this change tested?

Run two jaegertracing/jaeger-remote-storage at 17271 and 17281 ports
Execute go run -tags=ui ./cmd/jaeger --config ./cmd/jaeger/grpc_config.yaml

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

codecov · 2024-02-24T10:58:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.62%. Comparing base (0c53139) to head (cd405ef).

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5228       +/-   ##
===========================================
+ Coverage   52.95%   94.62%   +41.67%     
===========================================
  Files         149      336      +187     
  Lines        8796    19521    +10725     
===========================================
+ Hits         4658    18472    +13814     
+ Misses       3779      861     -2918     
+ Partials      359      188      -171

Flag	Coverage Δ
cassandra-3.x	`24.94% <ø> (ø)`
cassandra-4.x	`24.94% <ø> (ø)`
elasticsearch-5.x	`21.66% <ø> (ø)`
elasticsearch-6.x	`21.66% <ø> (ø)`
elasticsearch-7.x	`21.79% <ø> (ø)`
elasticsearch-8.x	`21.88% <ø> (ø)`
grpc-badger	`19.26% <42.85%> (+0.15%)`	⬆️
kafka	`13.74% <ø> (ø)`
opensearch-1.x	`21.79% <ø> (+0.01%)`	⬆️
opensearch-2.x	`21.79% <ø> (+0.01%)`	⬆️
unittests	`92.38% <72.72%> (?)`

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>

james-ryans · 2024-02-24T15:08:51Z

plugin/storage/grpc/config/config.go

@@ -78,6 +79,9 @@ func (c *Configuration) Close() error {
 		c.pluginHealthCheck.Stop()
 		c.pluginHealthCheckDone <- true
 	}
+	if c.remoteRPCClient != nil {


To pass the previous code coverage, I set up a gRPC server within the unit test for the factory to use, since we can't just mock the gRPC configuration. Because the current gRPC storage doesn't close the remote gRPC client, I added a method to close the client right inside the factory's Close() function. However, it looks like the code coverage tool isn't recognizing this particular line as covered. Even though I've called the Close() function in my unit test, which should hit this line, the method is abstracted behind an interface. Could this be why it's not showing up as covered?

in Go coverage is computed in a weird way, it's package local, i.e. coverage is only collected for a package from tests running in the same package. This behavior can be altered by passing additional packages explicitly, as we do for storage integration tests (we tell Go to count coverage in all other packages):

$(GOTEST) -coverpkg=./... -coverprofile cover.out $(STORAGE_PKGS)

So when the integration test runs for grpc-plugin it should include your Close changes, but not if you just run go test manually without -coverpkg

yurishkuro · 2024-02-24T22:41:16Z

plugin/storage/grpc/factory_test.go

+	if err != nil {
+		t.Fatalf("failed to listen: %v", err)
+	}


Suggested change

if err != nil {

t.Fatalf("failed to listen: %v", err)

}

require.NoError(t, err, "failed to listen")

yurishkuro · 2024-02-24T22:41:40Z

plugin/storage/grpc/config/config.go

@@ -106,12 +110,13 @@ func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.Tra
 		opts = append(opts, grpc.WithUnaryInterceptor(tenancy.NewClientUnaryInterceptor(tenancyMgr)))
 		opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr)))
 	}
-	conn, err := grpc.DialContext(ctx, c.RemoteServerAddr, opts...)
+	var err error
+	c.remoteRPCClient, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...)


this is not a client

Suggested change

c.remoteRPCClient, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...)

c.remoteConn, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...)

yurishkuro · 2024-02-24T22:46:45Z

plugin/storage/grpc/config/config.go

@@ -78,6 +79,9 @@ func (c *Configuration) Close() error {
 		c.pluginHealthCheck.Stop()
 		c.pluginHealthCheckDone <- true
 	}
+	if c.remoteRPCClient != nil {


in Go coverage is computed in a weird way, it's package local, i.e. coverage is only collected for a package from tests running in the same package. This behavior can be altered by passing additional packages explicitly, as we do for storage integration tests (we tell Go to count coverage in all other packages):

$(GOTEST) -coverpkg=./... -coverprofile cover.out $(STORAGE_PKGS)

So when the integration test runs for grpc-plugin it should include your Close changes, but not if you just run go test manually without -coverpkg

yurishkuro · 2024-02-24T22:47:47Z

cmd/jaeger/internal/extension/jaegerstorage/config.go

 )

 // Config has the configuration for jaeger-query,
 type Config struct {
 	Memory map[string]memoryCfg.Configuration   `mapstructure:"memory"`
 	Badger map[string]badgerCfg.NamespaceConfig `mapstructure:"badger"`
+	GRPC   map[string]grpcCfg.Configuration     `mapstructure:"grpc"`


I don't think this is how we want to do the configuration in V2, but we can address it more holistically later -- #5229

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

yurishkuro · 2024-02-27T15:41:57Z

plugin/storage/grpc/factory_test.go

+		err := f.Close()
+		if err != nil {
+			log.Fatalf("Client exited with error: %v", err)
+		}


Suggested change

err := f.Close()

if err != nil {

log.Fatalf("Client exited with error: %v", err)

}

require.NoError(t, f.Close())

plugin/storage/grpc/factory_test.go

yurishkuro · 2024-02-27T15:45:06Z

plugin/storage/integration/grpc_test.go

 }

 func (s *GRPCStorageIntegrationTestSuite) initialize() error {
 	s.logger, _ = testutils.NewLogger()

+	if s.factory != nil {


how would it be not nil at this point if the factory is only created in L119? Is it from another run of the test? If so we should close it as part of the clean-up, not as part of starting a new test

Okay, my initial thought was because it restarts the server when initializing, so I think it makes sense if I put it there too. But, moving it to the clean-up process does make things clearer. I've changed it.

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

james-ryans · 2024-02-27T16:22:28Z

Do you know why the previous Protogen Validation CI failed? I didn't change anything in the idl module, and now it succeeded without me fixing anything. https://github.com/jaegertracing/jaeger/actions/runs/8066523840

yurishkuro · 2024-02-27T16:40:09Z

Unclear why it failed:

--gogo_out: protoc-gen-gogo: Plugin failed with status code 127.
make: *** [Makefile.Protobuf.mk:97: proto-api-v2] Error 1

It's strange that the container doesn't print any more logs about it.

feat: add support for grpc in jaeger-v2

ee39180

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

james-ryans requested a review from a team as a code owner February 24, 2024 10:50

james-ryans requested a review from albertteoh February 24, 2024 10:50

test: cover a success NewFactoryWithConfig line

2798a45

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

james-ryans commented Feb 24, 2024

View reviewed changes

yurishkuro added changelog:exprimental Change to an experimental part of the code v2 labels Feb 24, 2024

yurishkuro mentioned this pull request Feb 24, 2024

[jaeger-v2] Rethink storage configuration to align with OTEL #5229

Open

yurishkuro reviewed Feb 24, 2024

View reviewed changes

james-ryans added 4 commits February 27, 2024 21:06

refactor remoteConn naming and factory_test require.NoError()

709bb09

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

add coverage to grpc.Configuration Close() from integration test

f50daec

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

Merge branch 'main' into grpc-storage-v2

c65ddfc

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

fix unsaved merge conflict in jaeger storage config

05d2c05

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

yurishkuro reviewed Feb 27, 2024

View reviewed changes

plugin/storage/grpc/factory_test.go Show resolved Hide resolved

yurishkuro reviewed Feb 27, 2024

View reviewed changes

james-ryans added 2 commits February 27, 2024 23:01

refactor factory close from grpc_test to clean-up

992b223

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

refactor grpc factory_test

cd405ef

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

yurishkuro approved these changes Feb 27, 2024

View reviewed changes

yurishkuro merged commit 751efdb into jaegertracing:main Feb 27, 2024
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jaeger-v2] Add support for GRPC storarge #5228

[jaeger-v2] Add support for GRPC storarge #5228

james-ryans commented Feb 24, 2024

codecov bot commented Feb 24, 2024 •

edited

james-ryans Feb 24, 2024 •

edited

yurishkuro Feb 24, 2024

yurishkuro Feb 24, 2024

yurishkuro Feb 24, 2024

yurishkuro Feb 24, 2024

yurishkuro Feb 24, 2024

yurishkuro Feb 27, 2024

yurishkuro Feb 27, 2024

james-ryans Feb 27, 2024

james-ryans commented Feb 27, 2024

yurishkuro commented Feb 27, 2024

	c.remoteRPCClient, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...)
	c.remoteConn, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...)

[jaeger-v2] Add support for GRPC storarge #5228

[jaeger-v2] Add support for GRPC storarge #5228

Conversation

james-ryans commented Feb 24, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Feb 24, 2024 • edited

Codecov Report

james-ryans Feb 24, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-ryans commented Feb 27, 2024

yurishkuro commented Feb 27, 2024

codecov bot commented Feb 24, 2024 •

edited

james-ryans Feb 24, 2024 •

edited