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

[WIP] Remove dependency on ArchiveFactory #6156

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

[WIP] Remove dependency on ArchiveFactory #6156

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
92cc3f4
Add IsArchive Flag To ESConfig
mahadzaryab1 Nov 3, 2024
8a548a1
Change Options To Only Hold One Config
mahadzaryab1 Nov 3, 2024
dd04a13
Change ES Factory To Remove Archive Implementation
mahadzaryab1 Nov 3, 2024
209b834
Change Usages of Factory
mahadzaryab1 Nov 3, 2024
89c7c70
Remove Namespace Parameter For NewFactoryWithConfig
mahadzaryab1 Nov 3, 2024
c6b6274
Fix Build Errors And Naming
mahadzaryab1 Nov 3, 2024
92e8070
Remove Cast To Archive Factory
mahadzaryab1 Nov 3, 2024
535aa2c
Fix Unit Test
mahadzaryab1 Nov 3, 2024
76a3528
Remove Archive Reader And Writers From Blackhole And Memory Storage
mahadzaryab1 Nov 3, 2024
50795b0
Fix Lint Checks
mahadzaryab1 Nov 3, 2024
a4d41b9
Fix Unit Test
mahadzaryab1 Nov 3, 2024
11a560d
Fix Unit Tests
mahadzaryab1 Nov 4, 2024
8c8e8ae
Fix Lint Error And Comment Out Archive Flags
mahadzaryab1 Nov 4, 2024
bbcdbd7
Initialize Archive Factory In Tests
mahadzaryab1 Nov 4, 2024
6c1fd95
Set IsArchive Flag In Factory
mahadzaryab1 Nov 4, 2024
43487cd
Fix Span Writer In ES Config
mahadzaryab1 Nov 4, 2024
c65bd21
Merge branch 'main' into refactor-archive-es
mahadzaryab1 Nov 4, 2024
7f87200
Expose is_archive Flag
mahadzaryab1 Nov 4, 2024
7d65eb8
Refactor Cassandra Namespace To Only Hold One Storage Type
mahadzaryab1 Nov 9, 2024
c385416
Fix Cassandra Archive Integration Test
mahadzaryab1 Nov 9, 2024
4194aab
Fix Linter
mahadzaryab1 Nov 9, 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
12 changes: 0 additions & 12 deletions 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 @@ -278,18 +278,6 @@ func TestServerAddArchiveStorage(t *testing.T) {
expectedOutput: "",
expectedErr: "cannot find archive storage factory: cannot find extension",
},
{
name: "Archive storage not supported",
config: &Config{
Storage: Storage{
TracesArchive: "badger",
},
},
qSvcOpts: &querysvc.QueryServiceOptions{},
extension: fakeStorageExt{},
expectedOutput: "Archive storage not supported by the factory",
expectedErr: "",
},
}

for _, tt := range tests {
Expand Down
20 changes: 4 additions & 16 deletions cmd/query/app/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,24 +92,12 @@ func TestBuildQueryServiceOptions(t *testing.T) {
require.NoError(t, err)
assert.NotNil(t, qOpts)

qSvcOpts := qOpts.BuildQueryServiceOptions(&mocks.Factory{}, zap.NewNop())
assert.NotNil(t, qSvcOpts)
assert.NotNil(t, qSvcOpts.Adjuster)
assert.Nil(t, qSvcOpts.ArchiveSpanReader)
assert.Nil(t, qSvcOpts.ArchiveSpanWriter)

comboFactory := struct {
*mocks.Factory
*mocks.ArchiveFactory
}{
&mocks.Factory{},
&mocks.ArchiveFactory{},
}
factory := &mocks.Factory{}

comboFactory.ArchiveFactory.On("CreateArchiveSpanReader").Return(&spanstore_mocks.Reader{}, nil)
comboFactory.ArchiveFactory.On("CreateArchiveSpanWriter").Return(&spanstore_mocks.Writer{}, nil)
factory.On("CreateSpanReader").Return(&spanstore_mocks.Reader{}, nil)
factory.On("CreateSpanWriter").Return(&spanstore_mocks.Writer{}, nil)

qSvcOpts = qOpts.BuildQueryServiceOptions(comboFactory, zap.NewNop())
qSvcOpts := qOpts.BuildQueryServiceOptions(factory, zap.NewNop())
assert.NotNil(t, qSvcOpts)
assert.NotNil(t, qSvcOpts.Adjuster)
assert.NotNil(t, qSvcOpts.ArchiveSpanReader)
Expand Down
17 changes: 2 additions & 15 deletions cmd/query/app/querysvc/query_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,25 +128,12 @@ func (qs QueryService) GetCapabilities() StorageCapabilities {

// InitArchiveStorage tries to initialize archive storage reader/writer if storage factory supports them.
func (opts *QueryServiceOptions) InitArchiveStorage(storageFactory storage.Factory, logger *zap.Logger) bool {
archiveFactory, ok := storageFactory.(storage.ArchiveFactory)
if !ok {
logger.Info("Archive storage not supported by the factory")
return false
}
reader, err := archiveFactory.CreateArchiveSpanReader()
if errors.Is(err, storage.ErrArchiveStorageNotConfigured) || errors.Is(err, storage.ErrArchiveStorageNotSupported) {
logger.Info("Archive storage not created", zap.String("reason", err.Error()))
return false
}
reader, err := storageFactory.CreateSpanReader()
if err != nil {
logger.Error("Cannot init archive storage reader", zap.Error(err))
return false
}
writer, err := archiveFactory.CreateArchiveSpanWriter()
if errors.Is(err, storage.ErrArchiveStorageNotConfigured) || errors.Is(err, storage.ErrArchiveStorageNotSupported) {
logger.Info("Archive storage not created", zap.String("reason", err.Error()))
return false
}
writer, err := storageFactory.CreateSpanWriter()
if err != nil {
logger.Error("Cannot init archive storage writer", zap.Error(err))
return false
Expand Down
17 changes: 4 additions & 13 deletions cmd/query/app/querysvc/query_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,31 +315,22 @@ func (*fakeStorageFactory1) CreateSpanReader() (spanstore.Reader, error)
func (*fakeStorageFactory1) CreateSpanWriter() (spanstore.Writer, error) { return nil, nil }
func (*fakeStorageFactory1) CreateDependencyReader() (dependencystore.Reader, error) { return nil, nil }

func (f *fakeStorageFactory2) CreateArchiveSpanReader() (spanstore.Reader, error) { return f.r, f.rErr }
func (f *fakeStorageFactory2) CreateArchiveSpanWriter() (spanstore.Writer, error) { return f.w, f.wErr }
func (f *fakeStorageFactory2) CreateSpanReader() (spanstore.Reader, error) { return f.r, f.rErr }
func (f *fakeStorageFactory2) CreateSpanWriter() (spanstore.Writer, error) { return f.w, f.wErr }

var (
_ storage.Factory = new(fakeStorageFactory1)
_ storage.ArchiveFactory = new(fakeStorageFactory2)
_ storage.Factory = new(fakeStorageFactory1)
_ storage.Factory = new(fakeStorageFactory2)
)

func TestInitArchiveStorageErrors(t *testing.T) {
opts := &QueryServiceOptions{}
logger := zap.NewNop()

assert.False(t, opts.InitArchiveStorage(new(fakeStorageFactory1), logger))
assert.False(t, opts.InitArchiveStorage(
&fakeStorageFactory2{rErr: storage.ErrArchiveStorageNotConfigured},
logger,
))
assert.False(t, opts.InitArchiveStorage(
&fakeStorageFactory2{rErr: errors.New("error")},
logger,
))
assert.False(t, opts.InitArchiveStorage(
&fakeStorageFactory2{wErr: storage.ErrArchiveStorageNotConfigured},
logger,
))
assert.False(t, opts.InitArchiveStorage(
&fakeStorageFactory2{wErr: errors.New("error")},
logger,
Expand Down
2 changes: 1 addition & 1 deletion cmd/query/app/token_propagation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func runQueryService(t *testing.T, esURL string) *Server {
flagsSvc := flags.NewService(ports.QueryAdminHTTP)
flagsSvc.Logger = zaptest.NewLogger(t)

f := es.NewFactory()
f := es.NewFactory(es.PrimaryNamespace)
v, command := config.Viperize(f.AddFlags)
require.NoError(t, command.ParseFlags([]string{
"--es.tls.enabled=false",
Expand Down
5 changes: 3 additions & 2 deletions cmd/remote-storage/app/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func NewServer(options *Options, storageFactory storage.Factory, tm *tenancy.Man
}, nil
}

func createGRPCHandler(f storage.Factory, logger *zap.Logger) (*shared.GRPCHandler, error) {
func createGRPCHandler(f storage.Factory, _ *zap.Logger) (*shared.GRPCHandler, error) {
reader, err := f.CreateSpanReader()
if err != nil {
return nil, err
Expand All @@ -78,7 +78,8 @@ func createGRPCHandler(f storage.Factory, logger *zap.Logger) (*shared.GRPCHandl
// borrow code from Query service for archive storage
qOpts := &querysvc.QueryServiceOptions{}
// when archive storage not initialized (returns false), the reader/writer will be nil
_ = qOpts.InitArchiveStorage(f, logger)
// TODO: what should we do here?
// _ = qOpts.InitArchiveStorage(f, logger)
Comment on lines +81 to +82
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 any thoughts on how we should handle this case here? previously, this wouldn't initialize the archive storage if the factory didn't implement the ArchiveStorage interface but now it always will.

Copy link
Member

Choose a reason for hiding this comment

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

This should not be different from all other v1 binaries. If I run

SPAN_STORAGE_TYPE=cassandra go run ./cmd/remote-storage help

I see that it supports the same --cassandra.* and --cassandra-archive.* flags, so we have enough information to instantiate two separate factories, just like v1 all-in-one/query would have to do.

impl.ArchiveSpanReader = func() spanstore.Reader { return qOpts.ArchiveSpanReader }
impl.ArchiveSpanWriter = func() spanstore.Writer { return qOpts.ArchiveSpanWriter }

Expand Down
2 changes: 2 additions & 0 deletions pkg/es/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ type Configuration struct {
Tags TagsAsFields `mapstructure:"tags_as_fields"`
// Enabled, if set to true, enables the namespace for storage pointed to by this configuration.
Enabled bool `mapstructure:"-"`
Copy link
Member

Choose a reason for hiding this comment

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

is this still used? Let's add some comments, because this is not the first time I have this question, and the flag doesn't really make sense to me.

// TODO: revisit if this needed
IsArchive bool
}

// TagsAsFields holds configuration for tag schema.
Expand Down
16 changes: 2 additions & 14 deletions plugin/storage/blackhole/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@ import (
"github.com/jaegertracing/jaeger/storage/spanstore"
)

var ( // interface comformance checks
_ storage.Factory = (*Factory)(nil)
_ storage.ArchiveFactory = (*Factory)(nil)
)
// interface comformance checks
var _ storage.Factory = (*Factory)(nil)

// Factory implements storage.Factory and creates blackhole storage components.
type Factory struct {
Expand Down Expand Up @@ -48,16 +46,6 @@ func (f *Factory) CreateSpanWriter() (spanstore.Writer, error) {
return f.store, nil
}

// CreateArchiveSpanReader implements storage.ArchiveFactory
func (f *Factory) CreateArchiveSpanReader() (spanstore.Reader, error) {
return f.store, nil
}

// CreateArchiveSpanWriter implements storage.ArchiveFactory
func (f *Factory) CreateArchiveSpanWriter() (spanstore.Writer, error) {
return f.store, nil
}

// CreateDependencyReader implements storage.Factory
func (f *Factory) CreateDependencyReader() (dependencystore.Reader, error) {
return f.store, nil
Expand Down
6 changes: 0 additions & 6 deletions plugin/storage/blackhole/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,6 @@ func TestStorageFactory(t *testing.T) {
writer, err := f.CreateSpanWriter()
require.NoError(t, err)
assert.Equal(t, f.store, writer)
reader, err = f.CreateArchiveSpanReader()
require.NoError(t, err)
assert.Equal(t, f.store, reader)
writer, err = f.CreateArchiveSpanWriter()
require.NoError(t, err)
assert.Equal(t, f.store, writer)
depReader, err := f.CreateDependencyReader()
require.NoError(t, err)
assert.Equal(t, f.store, depReader)
Expand Down
Loading
Loading