-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v2][storage] Add dependency store to v2 storage interface #6297
Changes from 17 commits
20c9702
8c00ee1
2fb8fc8
1ce9ffe
81010bf
7aae69b
934ca7a
a5e470d
daae5b9
424d845
9ce6576
e395f52
d07eebc
aa112ed
bcdc86d
55d3d19
f43ec56
4254ab4
38d0807
49de2c2
47d5f2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
"github.com/jaegertracing/jaeger/plugin/storage/grpc" | ||
"github.com/jaegertracing/jaeger/plugin/storage/memory" | ||
"github.com/jaegertracing/jaeger/storage" | ||
"github.com/jaegertracing/jaeger/storage_v2/depstore" | ||
"github.com/jaegertracing/jaeger/storage_v2/factoryadapter" | ||
"github.com/jaegertracing/jaeger/storage_v2/tracestore" | ||
) | ||
|
@@ -74,7 +75,15 @@ | |
return mf, nil | ||
} | ||
|
||
func GetStorageFactoryV2(name string, host component.Host) (tracestore.Factory, error) { | ||
func GetTraceStoreFactory(name string, host component.Host) (tracestore.Factory, error) { | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return getV2FactoryAdapter(name, host) | ||
} | ||
|
||
func GetDependencyStoreFactory(name string, host component.Host) (depstore.Factory, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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). |
||
return getV2FactoryAdapter(name, host) | ||
} | ||
|
||
func getV2FactoryAdapter(name string, host component.Host) (*factoryadapter.Factory, error) { | ||
f, err := GetStorageFactory(name, host) | ||
if err != nil { | ||
return nil, err | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,11 +28,12 @@ import ( | |
"github.com/jaegertracing/jaeger/plugin/metricstore/disabled" | ||
"github.com/jaegertracing/jaeger/proto-gen/api_v2" | ||
"github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" | ||
depsmocks "github.com/jaegertracing/jaeger/storage/dependencystore/mocks" | ||
"github.com/jaegertracing/jaeger/storage/metricstore" | ||
metricsmocks "github.com/jaegertracing/jaeger/storage/metricstore/mocks" | ||
"github.com/jaegertracing/jaeger/storage/spanstore" | ||
spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks" | ||
"github.com/jaegertracing/jaeger/storage_v2/depstore" | ||
depsmocks "github.com/jaegertracing/jaeger/storage_v2/depstore/mocks" | ||
"github.com/jaegertracing/jaeger/storage_v2/factoryadapter" | ||
) | ||
|
||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is expectedEndTs offset from endTs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should it be called startTs perhaps? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it looks like a bug There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. opened a patch at #6324 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro should be good to go now |
||
server.depReader.On("GetDependencies", | ||
mock.Anything, // context.Context | ||
endTs.Add(time.Duration(-1)*defaultDependencyLookbackDuration), | ||
defaultDependencyLookbackDuration, | ||
depstore.QueryParameters{ | ||
StartTime: expectedEndTs.Add(-defaultDependencyLookbackDuration), | ||
EndTime: expectedEndTs, | ||
}, | ||
).Return(expectedDependencies, nil).Times(1) | ||
|
||
res, err := client.GetDependencies(context.Background(), &api_v2.GetDependenciesRequest{ | ||
|
@@ -529,11 +533,14 @@ func TestGetDependenciesSuccessGRPC(t *testing.T) { | |
func TestGetDependenciesFailureGRPC(t *testing.T) { | ||
withServerAndClient(t, func(server *grpcServer, client *grpcClient) { | ||
endTs := time.Now().UTC() | ||
server.depReader.On( | ||
"GetDependencies", | ||
expectedEndTs := endTs.Add(time.Duration(-1) * defaultDependencyLookbackDuration) | ||
server.depReader.On("GetDependencies", | ||
mock.Anything, // context.Context | ||
endTs.Add(time.Duration(-1)*defaultDependencyLookbackDuration), | ||
defaultDependencyLookbackDuration).Return(nil, errStorageGRPC).Times(1) | ||
depstore.QueryParameters{ | ||
StartTime: expectedEndTs.Add(-defaultDependencyLookbackDuration), | ||
EndTime: expectedEndTs, | ||
}, | ||
).Return(nil, errStorageGRPC).Times(1) | ||
|
||
_, err := client.GetDependencies(context.Background(), &api_v2.GetDependenciesRequest{ | ||
StartTime: endTs.Add(time.Duration(-1) * defaultDependencyLookbackDuration), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// Copyright (c) 2024 The Jaeger Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package depstore | ||
|
||
import "github.com/jaegertracing/jaeger/storage_v2" | ||
|
||
type Factory interface { | ||
storage_v2.FactoryBase | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
CreateDependencyReader() (Reader, error) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro for v1, the factory adapter is created directly so we have direct access to this function - is this okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes