[storage] Remove distinction between primary and `archive` storage intefaces #6567

mahadzaryab1 · 2025-01-18T14:22:33Z

Which problem is this PR solving?

Towards Phase out the distinction between primary and archive storage #6065

Description of the changes

This PR completely removes the interface storage.ArchiveFactory by refactoring all the storage implementations to remove the distinction between a primary and archive interface. Note that the concept of archive storage remains the same within Jaeger, we just now use the same interface to handle both primary and archive storages.

How was this change tested?

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: npm run lint and npm run test

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

codecov · 2025-01-18T16:42:22Z

Codecov Report

Attention: Patch coverage is 98.02956% with 4 lines in your changes missing coverage. Please review.

Project coverage is 96.16%. Comparing base (b04e0ba) to head (263315b).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
plugin/storage/factory.go	96.10%	2 Missing and 1 partial ⚠️
plugin/storage/es/factory.go	98.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6567      +/-   ##
==========================================
- Coverage   96.24%   96.16%   -0.08%     
==========================================
  Files         373      372       -1     
  Lines       21406    21198     -208     
==========================================
- Hits        20602    20385     -217     
- Misses        612      618       +6     
- Partials      192      195       +3

Flag	Coverage Δ
badger_v1	`10.94% <0.00%> (+0.30%)`	⬆️
badger_v2	`2.85% <0.00%> (+0.07%)`	⬆️
cassandra-4.x-v1-manual	`16.35% <20.83%> (-0.23%)`	⬇️
cassandra-4.x-v2-auto	`2.79% <0.00%> (+0.07%)`	⬆️
cassandra-4.x-v2-manual	`2.79% <0.00%> (+0.07%)`	⬆️
cassandra-5.x-v1-manual	`16.35% <20.83%> (-0.23%)`	⬇️
cassandra-5.x-v2-auto	`2.79% <0.00%> (+0.07%)`	⬆️
cassandra-5.x-v2-manual	`2.79% <0.00%> (+0.07%)`	⬆️
elasticsearch-6.x-v1	`20.19% <19.27%> (-0.18%)`	⬇️
elasticsearch-7.x-v1	`20.27% <19.27%> (-0.18%)`	⬇️
elasticsearch-8.x-v1	`20.44% <19.27%> (-0.16%)`	⬇️
elasticsearch-8.x-v2	`2.84% <0.00%> (-0.04%)`	⬇️
grpc_v1	`12.05% <26.56%> (-0.11%)`	⬇️
grpc_v2	`9.27% <23.83%> (+0.24%)`	⬆️
kafka-3.x-v1	`10.61% <0.00%> (+0.29%)`	⬆️
kafka-3.x-v2	`2.85% <0.00%> (+0.07%)`	⬆️
memory_v2	`2.85% <0.00%> (+0.08%)`	⬆️
opensearch-1.x-v1	`20.31% <19.27%> (-0.17%)`	⬇️
opensearch-2.x-v1	`20.32% <19.27%> (-0.17%)`	⬇️
opensearch-2.x-v2	`2.84% <0.00%> (+0.06%)`	⬆️
tailsampling-processor	`0.53% <0.00%> (+0.01%)`	⬆️
unittests	`95.04% <97.53%> (-0.06%)`	⬇️

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.

cmd/remote-storage/app/server.go

plugin/storage/cassandra/factory.go

plugin/storage/cassandra/options.go

plugin/storage/es/factory.go

yurishkuro · 2025-01-18T17:26:57Z

plugin/storage/es/factory.go

-
-func (f *Factory) getArchiveClient() es.Client {
-	if c := f.archiveClient.Load(); c != nil {
+	if c := f.client.Load(); c != nil {


(not for this PR) I am curious if we still need this indirection. The primary reason it was added was to support TLS cert reload. But we don't do file-watcher-based reloads anymore, so nothing will ever trigger resetting of the client, and the cert rotation should be supported via OTEL's configtls indirection (as confirmed by the unit tests where we simulate rotation of certs). One this PR is merged let's open a good-first-issue ticket.

oh, maybe we still use file-watcher for password file, then we'd keep this.

plugin/storage/es/factory.go

plugin/storage/grpc/shared/interface.go

plugin/storage/integration/elasticsearch_test.go

plugin/storage/integration/grpc_test.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2025-01-20T20:57:04Z

cmd/query/app/flags.go

@@ -138,10 +138,18 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) (*Q
 	return qOpts, nil
 }

+type archiveInitializer interface {


nit: since this is used as part of (potentially) public API BuildQueryServiceOptions, and since you only need one function, a better approach would be accepting a parameter of public type

type InitArchiveStorageFn func(logger *zap.Logger) (spanstore.Reader, spanstore.Writer)

it's also easier to test with a function

yurishkuro · 2025-01-20T21:01:41Z

pkg/es/config/config.go

@@ -114,7 +114,9 @@ type Configuration struct {
 	// UseReadWriteAliases, if set to true, will use read and write aliases for indices.
 	// Use this option with Elasticsearch rollover API. It requires an external component
 	// to create aliases before startup and then performing its management.
-	UseReadWriteAliases bool `mapstructure:"use_aliases"`
+	UseReadWriteAliases bool   `mapstructure:"use_aliases"`
+	ReadAliasSuffix     string `mapstructure:"-"`


should add comment why these are not exposed via mapstructure:

yurishkuro · 2025-01-20T21:02:15Z

plugin/configurable.go

+
+// DefaultConfigurable is an interface that can be implement by some storage implementations
+// to provide a way to inherit configuration settings from another factory.
+type DefaultConfigurable interface {


Inheritable?

yurishkuro · 2025-01-20T21:03:41Z

plugin/storage/cassandra/factory.go

 )

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


add Inheritable?

yurishkuro · 2025-01-20T21:21:52Z

plugin/storage/es/factory.go


-	primaryClient atomic.Pointer[es.Client]
-	archiveClient atomic.Pointer[es.Client]
+	client atomic.Pointer[es.Client]

 	watchers []*fswatcher.FSWatcher


does this still need to be a list, or just exactly one watcher? If one, and it's for a password, I would rename it to pwdFileWatcher

yurishkuro · 2025-01-20T21:27:45Z

plugin/storage/factory.go

+				errs = append(errs, err)
+			}
+		}
+	}
 	for _, storageType := range f.SpanWriterTypes {


Any reason why this shouldn't simply be looping through f.factories and f.archiveFactories and closing them all? Seems weird that it depends on external list of types, considering that some types are added implicitly. Unless the same storage can be added more than once.

yurishkuro · 2025-01-20T21:30:51Z

plugin/storage/factory.go

+			if role == "archive" {
+				if primaryFactory, ok := f.factories[kind]; ok {
+					if dc, ok := factory.(plugin.DefaultConfigurable); ok {
+						dc.InheritSettingsFrom(primaryFactory)


is the order here correct, i.e. inheriting settings after calling Initialize?

yurishkuro · 2025-01-20T21:43:23Z

plugin/storage/factory.go

+					if dc, ok := factory.(plugin.DefaultConfigurable); ok {
+						dc.InheritSettingsFrom(primaryFactory)
+					}
+				}


is the coverage gap here real or stale?

yurishkuro · 2025-01-20T21:46:30Z

plugin/storage/grpc/shared/grpc_client.go

@@ -72,14 +67,6 @@ func (c *GRPCClient) StreamingSpanWriter() spanstore.Writer {
 	return newStreamingSpanWriter(c.streamWriterClient)
 }

-func (c *GRPCClient) ArchiveSpanReader() spanstore.Reader {
-	return &archiveReader{client: c.archiveReaderClient}


are we deleting these internal archiveReader/archiveWriter types?

yurishkuro · 2025-01-20T21:49:52Z

plugin/storage/grpc/shared/grpc_handler.go

@@ -52,7 +52,7 @@ func NewGRPCHandler(impl *GRPCHandlerStorageImpl) *GRPCHandler {
 // NewGRPCHandler creates a handler given implementations grouped by plugin services.
 func NewGRPCHandlerWithPlugins(


do we need this? I don't see anything using this, I was expecting the remote-storage to utilize this framework, but if it doesn't then we can delete it.

(maybe in a prequel PR)

yurishkuro · 2025-01-20T21:52:59Z

plugin/storage/integration/cassandra_test.go

 		"--cassandra-archive.servers=127.0.0.1",
 		"--cassandra-archive.basic.allowed-authenticators=org.apache.cassandra.auth.PasswordAuthenticator",
 		"--cassandra-archive.password=" + password,
 		"--cassandra-archive.username=" + username,


could we drop these? The inheritance should take care of them.

yurishkuro · 2025-01-20T21:54:21Z

plugin/storage/integration/elasticsearch_test.go

+	af := s.initializeESFactory(t, []string{
+		"--es-archive.enabled=true",
+		fmt.Sprintf("--es-archive.tags-as-fields.all=%v", allTagsAsFields),
+		fmt.Sprintf("--es-archive.index-prefix=%v", indexPrefix),


this can be dropped, for inheritance?

yurishkuro · 2025-01-20T21:54:54Z

plugin/storage/integration/grpc_test.go

+		err := command.ParseFlags(s.flags)
+		require.NoError(t, err)


Suggested change

err := command.ParseFlags(s.flags)

require.NoError(t, err)

require.NoError(t, command.ParseFlags(s.flags))

yurishkuro · 2025-01-20T21:56:13Z

plugin/storage/integration/grpc_test.go

+	f := initFactory()
+	af := initFactory()


so this creates a gRPC proxy pointing to the same remote storage port? I.e. not really testing two separate storage namespaces? Maybe we can fix in the follow-up.

yurishkuro · 2025-01-20T22:04:27Z

cmd/remote-storage/app/server.go

-	impl.ArchiveSpanWriter = func() spanstore.Writer { return qOpts.ArchiveSpanWriter }
+	ar, aw := f.InitArchiveStorage(logger)
+	impl.ArchiveSpanReader = func() spanstore.Reader { return ar }
+	impl.ArchiveSpanWriter = func() spanstore.Writer { return aw }

 	handler := shared.NewGRPCHandler(impl)


The existing Remote Storage API v1 has dedicated methods for WriteSpan and ArchiveSpan, exposed on the same gRPC service / port. Do we actually want that? Because the grpc storage client no longer knows about primary/archive, so it has only one SpanWriter that always calls WriteSpan gRPC method.

mahadzaryab1 added 4 commits January 17, 2025 18:11

Change ES Options To Only Hold One Namespace

befc6bd

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Configuration Fields For Suffixes

ab1180b

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Adjust ES Factory To Hold One Config

e1d6f7d

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Change Meta Factory To Initialize Archive Separately

5172646

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 mentioned this pull request Jan 18, 2025

[WIP] Upgrade storage integration test: Use V2 Archive ReaderWriter #6489

Draft

6 tasks

mahadzaryab1 added 10 commits January 18, 2025 09:33

Phase Out Distinction In Cassandra Factory

820b360

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Comment

6dcd077

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Archive Factory From Blackhole

2dc7d19

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Archive Factory From Memory

00c105c

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Archive Helper From Adapter

b57e488

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Concept of Archive From GRPC Storage

e90efe7

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Usages of InitArchiveStorage

1780ea5

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Archive Factory

1468260

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Cassandra Integration Test

43b7604

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix ES Integration Test

7c4c43e

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added the changelog:bugfix-or-minor-feature label Jan 18, 2025

mahadzaryab1 added 2 commits January 18, 2025 11:26

Fix GRPC Integration Tests

0b2280a

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Typo

5a06df6

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>