[v2][storage] Add dependency store to v2 storage interface #6297

mahadzaryab1 · 2024-12-02T22:58:40Z

Which problem is this PR solving?

Towards Implement Storage API v2 that operates on OTLP batches #5079

Description of the changes

This PR creates a new factory in the depstore package for the DependencyReader and exposes a CreateDependencyReader function in the factoryadapter
This PR also changes some interface return types to structs because some structs implement multiple interfaces.
The new factory was integrated into the query service and the callsites were changed to use the v2 factory instead of the v1 factory to initialize the dependency reader.

How was this change tested?

CI / Unit Tests

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: Mahad Zaryab <[email protected]>

mahadzaryab1 · 2024-12-02T23:05:20Z

@yurishkuro Where should we add the CreateDependencyReader function to? The GetStorageFactoryV2 in the storage extension returns a tracestore.Factory so I see the following options:

Add CreateDependencyReader to tracestore.Factory. I don't think think this is a good idea because dependencies and traces are different concepts.
Create a storage_v2.Factory that composes a tracestore.Factory and depstore.Factory.
Refactor GetStorageFactoryV2 to something like GetTraceStoreFactory and then expose a new function called GetDepStoreFactory. This would allow us to keep our interfaces small and only get the interface that we need.

Let me know what you think and if you have any other ideas.

codecov · 2024-12-02T23:06:35Z

Codecov Report

Attention: Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.14%. Comparing base (723202b) to head (47d5f2c).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...md/jaeger/internal/extension/jaegerquery/server.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6297      +/-   ##
==========================================
+ Coverage   96.11%   96.14%   +0.02%     
==========================================
  Files         356      356              
  Lines       20476    20489      +13     
==========================================
+ Hits        19681    19699      +18     
+ Misses        607      603       -4     
+ Partials      188      187       -1

Flag	Coverage Δ
badger_v1	`8.82% <0.00%> (-0.02%)`	⬇️
badger_v2	`1.61% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v1-manual	`14.71% <0.00%> (-0.03%)`	⬇️
cassandra-4.x-v2-auto	`1.56% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v2-manual	`1.56% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v1-manual	`14.71% <0.00%> (-0.03%)`	⬇️
cassandra-5.x-v2-auto	`1.56% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v2-manual	`1.56% <0.00%> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`18.44% <0.00%> (-0.04%)`	⬇️
elasticsearch-7.x-v1	`18.52% <0.00%> (-0.02%)`	⬇️
elasticsearch-8.x-v1	`18.67% <0.00%> (-0.03%)`	⬇️
elasticsearch-8.x-v2	`1.61% <0.00%> (-0.01%)`	⬇️
grpc_v1	`10.28% <0.00%> (-0.02%)`	⬇️
grpc_v2	`7.83% <0.00%> (-0.03%)`	⬇️
kafka-v1	`8.51% <0.00%> (-0.02%)`	⬇️
kafka-v2	`1.61% <0.00%> (-0.01%)`	⬇️
memory_v2	`1.61% <0.00%> (-0.02%)`	⬇️
opensearch-1.x-v1	`18.57% <0.00%> (-0.03%)`	⬇️
opensearch-2.x-v1	`18.57% <0.00%> (-0.03%)`	⬇️
opensearch-2.x-v2	`1.61% <0.00%> (-0.01%)`	⬇️
tailsampling-processor	`0.45% <0.00%> (-0.01%)`	⬇️
unittests	`95.05% <89.28%> (+0.02%)`	⬆️

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.

yurishkuro · 2024-12-03T00:29:51Z

I like option 3. We always had a split brain on dependencies, it was defined in a different package but part of the same factory interface. +1 to move to a different interface.

Signed-off-by: Mahad Zaryab <[email protected]>

yurishkuro · 2024-12-06T02:38:00Z

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

@@ -74,7 +73,7 @@ func GetMetricStorageFactory(name string, host component.Host) (storage.MetricSt
 	return mf, nil
 }

-func GetStorageFactoryV2(name string, host component.Host) (tracestore.Factory, error) {
+func GetStorageFactoryV2(name string, host component.Host) (*factoryadapter.Factory, error) {


don't know about this - adapter is implementation detail, this function should not leak it

@yurishkuro I was trying to follow's Go's best practice here of returning structs instead of interfaces. If we want to keep the interface return, we'll need to define another function GetDependencyStoreFactory with a different return type (depstore.Reader) but it will do the exact same thing. Let me know what you think.

it's not the best practice in this case. The return-struct best practice applies to constructors. GetStorageFactoryV2 is not a constructor, it's an accessor that is supposed to return a storage interface. The caller of this function is not supposed to know about the implementation of that interface and accordingly should not be able to take dependency on the concrete returned type, which could be wider than the interface alone.

Sounds good - so should we have two accessors here (something like GetTraceStoreFactory and GetDependencyStoreFactory)? Their return types would be different but the implementations would be the same.

yes, we should. Implementations are only the same today, not in the future.

sounds good to me! done

storage_v2/depstore/reader.go

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 · 2024-12-07T03:08:56Z

cmd/jaeger/internal/extension/jaegerquery/server.go

+	df, err := jaegerstorage.GetDependencyStoreFactory(s.config.Storage.TracesPrimary, host)
+	if err != nil {
+		return fmt.Errorf("cannot find factory for dependency storage %s: %w", s.config.Storage.TracesPrimary, err)
+	}


@yurishkuro is this okay? we're using the traces storage to initialize the dependency storage factory

mahadzaryab1 · 2024-12-07T03:10:20Z

cmd/all-in-one/main.go

@@ -114,7 +114,7 @@ by default uses only in-memory database.`,
 			if err != nil {
 				logger.Fatal("Failed to create span writer", zap.Error(err))
 			}
-			dependencyReader, err := storageFactory.CreateDependencyReader()
+			dependencyReader, err := v2Factory.CreateDependencyReader()


@yurishkuro for v1, the factory adapter is created directly so we have direct access to this function - is this okay?

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

yurishkuro · 2024-12-07T20:13:56Z

cmd/query/app/grpc_handler_test.go

@@ -511,10 +512,13 @@ func TestGetDependenciesSuccessGRPC(t *testing.T) {
 	withServerAndClient(t, func(server *grpcServer, client *grpcClient) {
 		expectedDependencies := []model.DependencyLink{{Parent: "killer", Child: "queen", CallCount: 12}}
 		endTs := time.Now().UTC()
+		expectedEndTs := endTs.Add(time.Duration(-1) * defaultDependencyLookbackDuration)


why is expectedEndTs offset from endTs?

should it be called startTs perhaps?

also, does Add(-defaultDependencyLookbackDuration) work?

@yurishkuro Its a bit weird because here startTime is passed into endTs (https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/grpc_handler.go#L226). Do you know why that is? I thought it would be passed the endTs and then the lookback

it looks like a bug

opened a patch at #6324

@yurishkuro should be good to go now

Signed-off-by: Mahad Zaryab <[email protected]>

yurishkuro · 2024-12-07T22:19:59Z

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

+	return getV2FactoryAdapter(name, host)
+}
+
+func GetDependencyStoreFactory(name string, host component.Host) (depstore.Factory, error) {


I've been looking at missing test coverage and realized that it's hard to get error coverage for this code path. We have this Extension API:

type Extension interface { extension.Extension TraceStorageFactory(name string) (storage.Factory, bool) MetricStorageFactory(name string) (storage.MetricStoreFactory, bool) }

Shouldn't it be exposing v2 factory instead of v1? And either it should have deps-related method OR there shouldn't be a static function for deps and instead it should be a runtime interface check, as we discussed on the ticket.

Maybe this is transitional? I actually don't like that the adapter is being applied by the static functions - I think the right way would be to have the extension itself expose v2 API (instead of v1 API - we could get to v1 via downgreading, if needed).

Signed-off-by: Mahad Zaryab <[email protected]>

storage_v2/depstore/factory.go

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 added 2 commits December 2, 2024 17:56

Add Interface For Dependency Store Reader

20c9702

Signed-off-by: Mahad Zaryab <[email protected]>

Add package_test.go

8c00ee1

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Dec 2, 2024

yurishkuro approved these changes Dec 5, 2024

View reviewed changes

mahadzaryab1 added 4 commits December 5, 2024 19:43

Create Separate Factory For Dependency Store

2fb8fc8

Signed-off-by: Mahad Zaryab <[email protected]>

Change Return Type of Factory Adapter And Implement Depstore Factory

1ce9ffe

Signed-off-by: Mahad Zaryab <[email protected]>

Add Unit Tests

81010bf

Signed-off-by: Mahad Zaryab <[email protected]>

Add License

7aae69b

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 changed the title ~~[WIP][v2][storage] Add dependency store to v2 storage interface~~ [v2][storage] Add dependency store to v2 storage interface Dec 6, 2024

mahadzaryab1 and others added 2 commits December 5, 2024 19:57

Change Return Type of GetStorageFactoryV2

934ca7a

Signed-off-by: Mahad Zaryab <[email protected]>

Merge branch 'main' into v2-depreader

a5e470d

mahadzaryab1 marked this pull request as ready for review December 6, 2024 01:10

mahadzaryab1 requested a review from a team as a code owner December 6, 2024 01:10

mahadzaryab1 requested a review from pavolloffay December 6, 2024 01:10

dosubot bot added area/storage v2 labels Dec 6, 2024

yurishkuro reviewed Dec 6, 2024

View reviewed changes

storage_v2/depstore/reader.go Outdated Show resolved Hide resolved

mahadzaryab1 added 3 commits December 6, 2024 16:58

Change Reader Interface To Take Struct Of Parameters

daae5b9

Signed-off-by: Mahad Zaryab <[email protected]>

Implement Dependency Reader Wrapper

424d845

Signed-off-by: Mahad Zaryab <[email protected]>

Update Factory To Use New Reader Wrapper

9ce6576

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 requested a review from yurishkuro December 6, 2024 22:12

mahadzaryab1 and others added 5 commits December 6, 2024 17:13

Merge branch 'main' into v2-depreader

e395f52

Generate Mocks For New Interface

d07eebc

Signed-off-by: Mahad Zaryab <[email protected]>

Use V2 Factory To Get Dependency Reader

aa112ed

Signed-off-by: Mahad Zaryab <[email protected]>

Fix Tests

bcdc86d

Signed-off-by: Mahad Zaryab <[email protected]>

Fix Tests

55d3d19

Signed-off-by: Mahad Zaryab <[email protected]>

Create Separate Accessors For Trace And Dependency Stores

f43ec56

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 commented Dec 7, 2024

View reviewed changes

yurishkuro reviewed Dec 7, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerstorage/extension.go Show resolved Hide resolved

yurishkuro reviewed Dec 7, 2024

View reviewed changes

mahadzaryab1 added 2 commits December 7, 2024 15:58

Merge branch 'main' into v2-depreader

4254ab4

Merge branch 'main' into v2-depreader

38d0807

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 requested a review from yurishkuro December 7, 2024 21:26

yurishkuro approved these changes Dec 7, 2024

View reviewed changes

yurishkuro reviewed Dec 7, 2024

View reviewed changes

Get Dependency Factory From Runtime Check

49de2c2

Signed-off-by: Mahad Zaryab <[email protected]>

yurishkuro reviewed Dec 7, 2024

View reviewed changes

storage_v2/depstore/factory.go Outdated Show resolved Hide resolved

Remove Base Factory

47d5f2c

Signed-off-by: Mahad Zaryab <[email protected]>

yurishkuro approved these changes Dec 7, 2024

View reviewed changes

yurishkuro enabled auto-merge (squash) December 7, 2024 23:10

yurishkuro disabled auto-merge December 7, 2024 23:12

yurishkuro merged commit b264d2c into jaegertracing:main Dec 7, 2024
53 of 54 checks passed

mahadzaryab1 deleted the v2-depreader branch December 8, 2024 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2][storage] Add dependency store to v2 storage interface #6297

[v2][storage] Add dependency store to v2 storage interface #6297

mahadzaryab1 commented Dec 2, 2024 •

edited

Loading

mahadzaryab1 commented Dec 2, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

yurishkuro commented Dec 3, 2024

yurishkuro Dec 6, 2024

mahadzaryab1 Dec 6, 2024

yurishkuro Dec 7, 2024

mahadzaryab1 Dec 7, 2024

yurishkuro Dec 7, 2024

mahadzaryab1 Dec 7, 2024

mahadzaryab1 Dec 7, 2024

yurishkuro Dec 7, 2024

mahadzaryab1 Dec 7, 2024 •

edited

Loading

yurishkuro Dec 7, 2024

yurishkuro Dec 7, 2024

yurishkuro Dec 7, 2024

yurishkuro Dec 7, 2024

mahadzaryab1 Dec 7, 2024 •

edited

Loading

yurishkuro Dec 7, 2024

mahadzaryab1 Dec 7, 2024

mahadzaryab1 Dec 7, 2024

yurishkuro Dec 7, 2024

[v2][storage] Add dependency store to v2 storage interface #6297

[v2][storage] Add dependency store to v2 storage interface #6297

Conversation

mahadzaryab1 commented Dec 2, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

mahadzaryab1 commented Dec 2, 2024

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

yurishkuro commented Dec 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahadzaryab1 Dec 7, 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

Choose a reason for hiding this comment

mahadzaryab1 Dec 7, 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

Choose a reason for hiding this comment

mahadzaryab1 commented Dec 2, 2024 •

edited

Loading

codecov bot commented Dec 2, 2024 •

edited

Loading

mahadzaryab1 Dec 7, 2024 •

edited

Loading

mahadzaryab1 Dec 7, 2024 •

edited

Loading