Skip to content
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.

Sign up for GitHub

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

Merged
merged 21 commits into from
Dec 7, 2024
+307 −109
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
20c9702
Add Interface For Dependency Store Reader
mahadzaryab1 Dec 2, 2024
8c00ee1
Add package_test.go
mahadzaryab1 Dec 2, 2024
2fb8fc8
Create Separate Factory For Dependency Store
mahadzaryab1 Dec 6, 2024
1ce9ffe
Change Return Type of Factory Adapter And Implement Depstore Factory
mahadzaryab1 Dec 6, 2024
81010bf
Add Unit Tests
mahadzaryab1 Dec 6, 2024
7aae69b
Add License
mahadzaryab1 Dec 6, 2024
934ca7a
Change Return Type of GetStorageFactoryV2
mahadzaryab1 Dec 6, 2024
a5e470d
Merge branch 'main' into v2-depreader
mahadzaryab1 Dec 6, 2024
daae5b9
Change Reader Interface To Take Struct Of Parameters
mahadzaryab1 Dec 6, 2024
424d845
Implement Dependency Reader Wrapper
mahadzaryab1 Dec 6, 2024
9ce6576
Update Factory To Use New Reader Wrapper
mahadzaryab1 Dec 6, 2024
e395f52
Merge branch 'main' into v2-depreader
mahadzaryab1 Dec 6, 2024
d07eebc
Generate Mocks For New Interface
mahadzaryab1 Dec 6, 2024
aa112ed
Use V2 Factory To Get Dependency Reader
mahadzaryab1 Dec 6, 2024
bcdc86d
Fix Tests
mahadzaryab1 Dec 6, 2024
55d3d19
Fix Tests
mahadzaryab1 Dec 6, 2024
f43ec56
Create Separate Accessors For Trace And Dependency Stores
mahadzaryab1 Dec 7, 2024
4254ab4
Merge branch 'main' into v2-depreader
mahadzaryab1 Dec 7, 2024
38d0807
Merge branch 'main' into v2-depreader
mahadzaryab1 Dec 7, 2024
49de2c2
Get Dependency Factory From Runtime Check
mahadzaryab1 Dec 7, 2024
47d5f2c
Remove Base Factory
mahadzaryab1 Dec 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/jaeger/internal/exporters/storageexporter/exporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func newExporter(config *Config, otel component.TelemetrySettings) *storageExpor
}

func (exp *storageExporter) start(_ context.Context, host component.Host) error {
f, err := jaegerstorage.GetStorageFactoryV2(exp.config.TraceStorage, host)
f, err := jaegerstorage.GetTraceStoreFactory(exp.config.TraceStorage, host)
if err != nil {
return fmt.Errorf("cannot find storage factory: %w", err)
}
Expand Down
13 changes: 8 additions & 5 deletions cmd/jaeger/internal/extension/jaegerquery/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,20 @@
telset.Metrics = telset.Metrics.
Namespace(metrics.NSOptions{Name: "jaeger"}).
Namespace(metrics.NSOptions{Name: "query"})
f, err := jaegerstorage.GetStorageFactoryV2(s.config.Storage.TracesPrimary, host)
tf, err := jaegerstorage.GetTraceStoreFactory(s.config.Storage.TracesPrimary, host)
if err != nil {
return fmt.Errorf("cannot find factory for primary storage %s: %w", s.config.Storage.TracesPrimary, err)
return fmt.Errorf("cannot find factory for trace storage %s: %w", s.config.Storage.TracesPrimary, err)
}

traceReader, err := f.CreateTraceReader()
traceReader, err := tf.CreateTraceReader()
if err != nil {
return fmt.Errorf("cannot create trace reader: %w", err)
}

depReader, err := f.CreateDependencyReader()
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)

Check warning on line 85 in cmd/jaeger/internal/extension/jaegerquery/server.go

View check run for this annotation

Codecov / codecov/patch

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

Added line #L85 was not covered by tests
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok for now

depReader, err := df.CreateDependencyReader()
if err != nil {
return fmt.Errorf("cannot create dependencies reader: %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/jaeger/internal/extension/jaegerquery/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func TestServerStart(t *testing.T) {
TracesPrimary: "need-factory-error",
},
},
expectedErr: "cannot find factory for primary storage",
expectedErr: "cannot find factory for trace storage",
},
{
name: "span reader error",
Expand Down
12 changes: 11 additions & 1 deletion cmd/jaeger/internal/extension/jaegerstorage/extension.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
"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"
)

var _ Extension = (*storageExt)(nil)
Expand Down Expand Up @@ -73,7 +75,15 @@
return mf, nil
}

func GetStorageFactoryV2(name string, host component.Host) (*factoryadapter.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) {
Copy link
Member

Choose a reason for hiding this comment

The 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:

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).

return getV2FactoryAdapter(name, host)

Check warning on line 83 in cmd/jaeger/internal/extension/jaegerstorage/extension.go

View check run for this annotation

Codecov / codecov/patch

cmd/jaeger/internal/extension/jaegerstorage/extension.go#L82-L83 

Added lines #L82 - L83 were not covered by tests
}

func getV2FactoryAdapter(name string, host component.Host) (*factoryadapter.Factory, error) {
f, err := GetStorageFactory(name, host)
if err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions cmd/jaeger/internal/extension/jaegerstorage/extension_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func TestStorageFactoryBadShutdownError(t *testing.T) {

func TestGetFactoryV2Error(t *testing.T) {
host := componenttest.NewNopHost()
_, err := GetStorageFactoryV2("something", host)
_, err := GetTraceStoreFactory("something", host)
require.ErrorContains(t, err, "cannot find extension")
}

Expand All @@ -112,7 +112,7 @@ func TestGetFactory(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, f)

f2, err := GetStorageFactoryV2(name, host)
f2, err := GetTraceStoreFactory(name, host)
require.NoError(t, err)
require.NotNil(t, f2)

Expand Down
Loading