[jaeger-v2] Add GRPC Unit Test #5306

Pushkarm029 · 2024-03-28T08:36:33Z

Which problem is this PR solving?

Part of [jaeger-v2] Storage backend integration tests #5254

Description of the changes

Resolves point 1 of the requirements.
Utilizes existing configuration files located in ./cmd/jaeger to initialize the storage extension.
Utilizes the storage extension to obtain a new storage factory.
Initializes necessary components such as spanReader, spanWriter, etc.
Executes test cases (located in plugin/storage/integration/integration.go) on remote storage utilizing these components.
done some refactoring to reuse multiple code segments from the existing testbed based integration test.

Difference b/w existing integration tests and v2 unit tests.

existing integration tests

directly, initializes grpc.Factory.

unit tests

starts storage extension. based on the config provided, currently in this test, we are reusing configs present in ./cmd/jaeger.
initializes storage.Factory using storage extension. which internally initializes grpc.Factory to create necessary components like spanReader, spanWriter.

How was this change tested?

./scripts/grpc-unit-test.sh latest

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: Pushkar Mishra <pushkarmishra029@gmail.com>

codecov · 2024-03-28T08:45:42Z

Codecov Report

Attention: Patch coverage is 5.17241% with 55 lines in your changes are missing coverage. Please review.

Project coverage is 94.44%. Comparing base (34f6a25) to head (875883b).
Report is 2 commits behind head on main.

Files	Patch %	Lines
cmd/jaeger/internal/integration/integration.go	6.52%	43 Missing ⚠️
cmd/jaeger/internal/integration/config.go	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5306      +/-   ##
==========================================
- Coverage   95.15%   94.44%   -0.71%     
==========================================
  Files         340      341       +1     
  Lines       16666    16716      +50     
==========================================
- Hits        15858    15788      -70     
- Misses        617      737     +120     
  Partials      191      191

Flag	Coverage Δ
badger	`12.25% <ø> (ø)`
cassandra-3.x	`21.14% <ø> (ø)`
cassandra-4.x	`21.14% <ø> (ø)`
elasticsearch-5.x	`17.70% <ø> (ø)`
elasticsearch-6.x	`17.68% <ø> (ø)`
elasticsearch-7.x	`17.76% <ø> (ø)`
elasticsearch-8.x	`17.82% <ø> (-0.02%)`	⬇️
grpc	`12.36% <ø> (+0.84%)`	⬆️
kafka	`11.84% <ø> (ø)`
opensearch-1.x	`17.75% <ø> (ø)`
opensearch-2.x	`17.76% <ø> (ø)`
unittests	`92.07% <5.17%> (-0.26%)`	⬇️

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.

Pushkarm029 · 2024-03-28T08:55:52Z

If this fulfils our 1st requirement (to keep integration tests operating as unit tests). Then, I will start working on the e2e test.

yurishkuro · 2024-03-28T14:28:22Z

not sure I follow. How is this different from plugin/storage/integration/grpc_test.go?

Pushkarm029 · 2024-03-28T15:43:04Z

not sure I follow. How is this different from plugin/storage/integration/grpc_test.go?

I copied it from there to make sure that when we add more tests, such as e2e, on top of it, they won't affect the v1 integration tests. Basically, to separate v1 and v2 tests. However, if we want to keep them in the same place, we can do so.

yurishkuro · 2024-03-28T16:17:38Z

There is no reason to duplicate the code. The existing integration tests are specific to Jaeger overall, in how it uses storage backends, they are not specific to v1 or v2 architecture. The real task is this one:

abstract how integration tests write and read span data

I added diagrams #5254 (comment)

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

yurishkuro · 2024-03-31T18:04:53Z

Makefile

@@ -5,6 +5,7 @@ SHELL := /bin/bash
 JAEGER_IMPORT_PATH = github.com/jaegertracing/jaeger
 STORAGE_PKGS = ./plugin/storage/integration/...
 JAEGER_STORAGE_PKGS = ./cmd/jaeger/internal/integration
+JAEGER_UNIT_STORAGE_PKGS = ./cmd/jaeger/internal/unittest


Please rebase on main, this is already in the makefile

why not put this into ./cmd/jaeger/internal/integration?

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

yurishkuro · 2024-03-31T21:53:14Z

cmd/jaeger/internal/unittest/unittest.go

+		t.Skipf("Integration test against Jaeger-V2 %[1]s skipped; set STORAGE env var to %[1]s to run this", s.Name)
+	}
+	var v integration.StorageIntegration
+	factories, err := internal.Components()


@james-ryans if feels like there is a lot of repetition here compared to cmd/jaeger/internal/integration/integration.go.

plugin/storage/integration/integration.go

scripts/grpc-unit-test.sh

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

cmd/jaeger/internal/integration/integration.go

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

Pushkarm029 · 2024-04-02T22:09:56Z

cmd/jaeger/internal/integration/integration.go

+func (s *StorageIntegration) cleanUp(t *testing.T) {
+	require.NoError(t, s.storageExtension.Shutdown(context.Background()))
+
+	time.Sleep(20 * time.Second)
+	s.initComponents(t)
+}


i think the issue is with the cleanup process. In the existing integration test, we simply close the factory and initialize it again. However, here, we don't have anything similar to close factory, so to tackle this, I tried the following approach -> in cleanup function

i shutted down the storage extension and then restarted it again. got this error,

Error: Received unexpected error: duplicate grpc storage name external-storage plugin error: rpc error: code = Canceled desc = grpc: the client connection is closing

yurishkuro · 2024-04-02T23:15:10Z

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

@@ -220,7 +220,7 @@ func makeStorageExtenion(t *testing.T, config *Config) component.Component {
 	return storageExtension
 }

-func startStorageExtension(t *testing.T, memstoreName string) component.Component {
+func StartStorageExtension(t *testing.T, memstoreName string) component.Component {


is this change necessary? I don't see this function being called from outside of this package (and if it were it would be a code smell, we don't share test code like that)

yurishkuro · 2024-04-02T23:16:38Z

cmd/jaeger/internal/integration/integration.go

-// the exporter TraceStorage name to set it to receiver TraceStorage.
-func (s *StorageIntegration) newDataReceiver(t *testing.T, factories otelcol.Factories) testbed.DataReceiver {
-	fmp := fileprovider.NewWithSettings(confmap.ProviderSettings{})
+func (s *StorageIntegration) initTelemetry(t *testing.T, factories otelcol.Factories) (*jaegerstorage.Config, *storageexporter.Config, component.TelemetrySettings) {


why does initTelemetry method return storage / exporter configs?

yurishkuro · 2024-04-02T23:20:10Z

I am really confused by this PR. Please explain in the description what it's trying to achieve. E.g. it says "A test has been added to perform test cases (present in plugin/storage/integration/integration.go) on remote storage by calling the storage API." - we already do that in plugin/storage/integration/grpc_test, what is different here? Explain the differences.

Pushkarm029 · 2024-04-03T12:05:12Z

I am really confused by this PR. Please explain in the description what it's trying to achieve. E.g. it says "A test has been added to perform test cases (present in plugin/storage/integration/integration.go) on remote storage by calling the storage API." - we already do that in plugin/storage/integration/grpc_test, what is different here? Explain the differences.

I apologize, I have just added it now. please check it.

james-ryans · 2024-04-03T13:02:21Z

cmd/jaeger/internal/integration/integration.go

+	factories, err := internal.Components()
+	require.NoError(t, err)
+
+	storageCfg, exporterCfg, telemetrySettings := s.initTelemetry(t, factories)


Looks like you're trying to get the configuration from the ./cmd/jaeger config file, it isn't necessary because this PR is a unit test mode and the scopes are only to solve (2), (3), and (5). The (4) requirement (use config from ./cmd/jaeger) can be covered by e2e test mode.

You're allowed to initialize the jaeger storage manually with your own defined config, which might be more readable. Also, this initialization step should be clearer if it is moved to initComponents too.

I agree, and if we ignore the config file part because it will be exercised by e2e test, then I still don't see what else this PR is testing that is not already tested by the existing integration tests. I showed two diagrams in the ticket - this PR doesn't fit either of them.

Oh, I thought the second point in the ticket stated that "unit mode" and "e2e mode" refer to v2's storage integration tests, so we need to create both "unit" and "e2e" tests for the v2 storage backend. This PR represents the unit mode for that, by reading/writing span data through the Jaeger storage extension.
The second point of the ticket states:

we can abstract how integration tests write and read span data, such that in the unit test mode they would call storage API as a library, but in e2e mode they will do the same via RPC requests.

After giving it some thought, this PR only gives an additional benefit to solve point (5) that "generate code coverage for some parts of the code that do not get exercised in unit tests", and perhaps also indirectly solves (2) and (3) since this extension is somewhat a wrapper to storage API. But I realized maybe what you meant by unit mode is exactly the existing integration tests (?), since points (2) and (3) are already solved by them and point (5) will be covered by e2e mode.

So, do you think we still need to add the existing integration test to test through the Jaeger storage extension? Not through the whole collector pipeline nor directly to the storage API.

do you think we still need to add the existing integration test to test through the Jaeger storage extension?

The logic in the storage extension can be unit-tested directly, because all the extension does is delegate to factories. It already has 100% coverage:

$ go test -cover ./cmd/jaeger/internal/extension/jaegerstorage/ ok github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerstorage 6.055s coverage: 100.0% of statements

So yes, I did mean we should reuse the existing integration tests. My comment said:

Our storage integration tests solve (2), (3) and (5), and I think (6)

My proposal was to extend (not duplicate!) those integration tests to also enable e2e testing, by replacing direct function calls to storage API with RPC calls to the separately running collector/query pipeline. And I think it's preferable if that separately running pipeline is actually running the real jaeger-v2 binary, instead of being orchestrated in-process by the unit tests themselves, because then it wouldn't be an e2e test.

Pushkarm029 · 2024-04-03T18:57:27Z

so, there were some misunderstandings. this pr does not solve anything.

point 1

f3521e5

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

yurishkuro added the changelog:test Change that's adding missing tests or correcting existing tests label Mar 28, 2024

Pushkarm029 added 2 commits March 30, 2024 14:00

Merge branch 'main' into integration_test

0469085

fix

a25523f

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

yurishkuro reviewed Mar 31, 2024

View reviewed changes

Pushkarm029 added 3 commits April 1, 2024 00:30

Merge branch 'main' into integration_test

4848e67

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

refactoring done

4199663

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

pointing both main and archive to same server

87b1c2c

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

Pushkarm029 marked this pull request as ready for review March 31, 2024 19:56

Pushkarm029 requested a review from a team as a code owner March 31, 2024 19:56

Pushkarm029 requested a review from jkowall March 31, 2024 19:56

yurishkuro reviewed Mar 31, 2024

View reviewed changes

fix

875883b

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

Pushkarm029 commented Apr 1, 2024

View reviewed changes

cmd/jaeger/internal/integration/integration.go Outdated Show resolved Hide resolved

Pushkarm029 added 2 commits April 3, 2024 03:25

Merge branch 'main' into integration_test

66af073

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

error in refresh func

ce88482

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>

Pushkarm029 commented Apr 2, 2024

View reviewed changes

yurishkuro reviewed Apr 2, 2024

View reviewed changes

james-ryans reviewed Apr 3, 2024

View reviewed changes

Pushkarm029 closed this Apr 3, 2024

Pushkarm029 deleted the integration_test branch May 22, 2024 05:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jaeger-v2] Add GRPC Unit Test #5306

[jaeger-v2] Add GRPC Unit Test #5306

Pushkarm029 commented Mar 28, 2024 •

edited

Loading

codecov bot commented Mar 28, 2024 •

edited

Loading

Pushkarm029 commented Mar 28, 2024

yurishkuro commented Mar 28, 2024

Pushkarm029 commented Mar 28, 2024

yurishkuro commented Mar 28, 2024

yurishkuro Mar 31, 2024

yurishkuro Mar 31, 2024

yurishkuro Mar 31, 2024

Pushkarm029 Apr 2, 2024

yurishkuro Apr 2, 2024

yurishkuro Apr 2, 2024

yurishkuro commented Apr 2, 2024

Pushkarm029 commented Apr 3, 2024

james-ryans Apr 3, 2024 •

edited

Loading

yurishkuro Apr 3, 2024

james-ryans Apr 3, 2024

yurishkuro Apr 3, 2024

Pushkarm029 commented Apr 3, 2024

[jaeger-v2] Add GRPC Unit Test #5306

[jaeger-v2] Add GRPC Unit Test #5306

Conversation

Pushkarm029 commented Mar 28, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

Difference b/w existing integration tests and v2 unit tests.

existing integration tests

unit tests

How was this change tested?

Checklist

codecov bot commented Mar 28, 2024 • edited Loading

Codecov Report

Pushkarm029 commented Mar 28, 2024

yurishkuro commented Mar 28, 2024

Pushkarm029 commented Mar 28, 2024

yurishkuro commented Mar 28, 2024

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

yurishkuro commented Apr 2, 2024

Pushkarm029 commented Apr 3, 2024

james-ryans Apr 3, 2024 • edited Loading

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

Pushkarm029 commented Apr 3, 2024

Pushkarm029 commented Mar 28, 2024 •

edited

Loading

codecov bot commented Mar 28, 2024 •

edited

Loading

james-ryans Apr 3, 2024 •

edited

Loading