Phase out the distinction between primary and archive storage

[yurishkuro]

Update Jan 7 2025: the intention of this issue is to do away with awareness in the code of the differences between primary and archive storage. The two implementations are generally identical, have identical reader/writer API, with only minor changes in behavior that can be managed with more explicit parameters instead if a hardcoded isArchive flag. For example, in the "legacy" mode the ES primary storage uses manual rotation of indices, e.g. by creating a new index name every rollover period, whereas archive storage always uses a fixed index name. The latter behavior can be reproduced by setting use_aliases flag instead of having an explicit understanding of isArchive mode.

This unification becomes especially important in Jaeger-v2 where storage backends are already declared uniformly in the jaeger_storage extension without any knowledge of primary/archive distinction. The distinction only appears once jaegerquery extension retrieves a specific backend by name and uses it as either primary or archive backend, by which point the backend itself is already initialized. The intent is to expose required configuration options to the user such that they can customize the behavior of the backend used for archive as they see fit, e.g. if they still want to rotate the ES indices, they can do so.

---

This thread #6060 (comment)

[mahadzaryab1]

@yurishkuro dug into this a little bit while redoing the configurations and here's what I found

• The factory creates a primary and archive namespace for each provided configuration (

  jaeger/plugin/storage/es/factory.go

  Lines 88 to 95 in f411b3c

  	defaultConfig := DefaultConfig()
  	cfg.ApplyDefaults(&defaultConfig)
  	archive := make(map[string]*namespaceConfig)
  	archive[archiveNamespace] = &namespaceConfig{
  	Configuration: cfg,
  	namespace: archiveNamespace,
  	}

• CreateArchiveSpanReader and CreateArchiveSpanWriter are called by InitArchiveStorage in v1's query service (https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/querysvc/query_service.go#L130)
• The InitArchiveStorage method is called by the v2 jaeger query extension (https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/internal/extension/jaegerquery/server.go#L137)

So I believe that this is working as expected. Let me know if there's anything I'm missing here.

One other thing to note is that in v1, the additional namespaces need to be explictly enabled (https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/es/options.go#L110) but the archive namespace is enabled by default in v2 (https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/es/options.go#L437).

[yurishkuro]

@mahadzaryab1 In v1 the workflow is:

1. the binary creates a Factory
2. asks factory to initialize itself from CLI flags
3. creates main storage implementations via CreateSpanWriter
4. optionally calls CreateArchiveSpanWriter

The key observation here is that the distinction between primary and archive is handled inside the Factory because the caller clearly indicates the intent by calling InitArchiveStorage and CreateArchiveSpanWriter. in other words, the caller delegates the specific initialization required for archive storage to the Factory - a single Factory that manages both primary and archive.

In contrast, in v2 the workflow is supposed to be different

• the user designates a configuration as primary or archive, but the configuration otherwise is the same and each configuration is represented by a unique Factory object
• when initializing query service the v2 extension is supposed to call CreateSpanWriter on the primary factory, and also call CreateSpanWriter on the archive factory (calling the same API)
• instead, v2 extension still falls back onto calling two different APIs due to invoking InitArchiveStorage

So v2 is serendipitously working right now, but not as expected. We can see it in how Memory storage is handled - when I configured all-in-one with primary and archive storage, originally it did not work for me because Memory storage factory never implemented the Archive sub-API, so I had to add it (and the side effect of it is that archive for memory cannot be turned off now). But that was exactly the opposite of what I wanted to happen - I wanted the config to be able to configure two storages and designate each as primary / archive, so that the v2 extension would only interact with the factories via CreateSpanWriter.

[mahadzaryab1]

@yurishkuro Thanks so much for the helpful context. Do you have any thoughts on how we should proceed here? Do we want to make changes to the query extension to not use InitArchiveStorage?

[yurishkuro]

Yes that would be ideal. It may need refactoring of query service, eg perhaps it should be taking a separate archive factory instead of using a different interface on the main factory.

[mahadzaryab1]

@yurishkuro just looking for a bit of clarification

so today, we're doing the following:

	f, err := jaegerstorage.GetStorageFactory(s.config.Storage.TracesArchive, host)
	if err != nil {
		return fmt.Errorf("cannot find archive storage factory: %w", err)
	}

	if !opts.InitArchiveStorage(f, s.telset.Logger) {
		s.telset.Logger.Info("Archive storage not initialized")
	}

Is the goal to be able to just do the following? Why do we need to introduce a new archive factory?

	f, err := jaegerstorage.GetStorageFactory(s.config.Storage.TracesArchive, host)
	if err != nil {
		return fmt.Errorf("cannot find archive storage factory: %w", err)
	}

	spanReader, err := f.CreateSpanReader()
	if err != nil {
		return fmt.Errorf("cannot create span reader: %w", err)
	}
        opts.ArchiveSpanReader = f.spanReader

[yurishkuro]

yes

[mahadzaryab1]

@yurishkuro do we still need a separate interface to do the above? One other question I had was, will simply calling CreateSpanReader behave the same way as CreateArchiveSpanReader does? How would there be a differentiation between normal storage logic and archive storage logic?

[yurishkuro]

That is a good question, but if you look at all implementations of GetArchiveSomething they are no different from regular method. I think the only difference is in ES implementation which needs the isArchive flag. We can expose that flag as part of the ES config - not ideal because the user has to remember to set it, otherwise they might get limited look back. But I think in ES the archive storage requires separate settings anyway, eg you may not want to rollover index every day.

[mahadzaryab1]

@yurishkuro How would it work if we add the is_archive flag to the ES config? We're currently storing separate configs in the ES factory as follows so we have no way of knowing if CreateSpanSomething is being called for the regular config or the archive config.

	primaryConfig *config.Configuration
	archiveConfig *config.Configuration

[mahadzaryab1]

Do we want to refactor the ES factory to only only hold one config/client and then propagate the isArchive flag to it?

[yurishkuro]

In v2 we have two independent factories. I'd say yes, we want to refactor the factories to represent just one kind of storage. It will require changes to query service. I think only Cassandra and ES factories are storing two different namespaces.

[mahadzaryab1]

@yurishkuro Got it! I'll get started on that. Thanks!

[yurishkuro]

This is not blocking for v2 GA, I confirmed that a proper code path is being invoked, e.g. by introducing a panic in CreateArchiveSpanReader:

$ go run ./cmd/jaeger --config ./cmd/jaeger/config-elasticsearch.yaml
...
panic: archive reader not implemented

goroutine 1 [running]:
github.com/jaegertracing/jaeger/plugin/storage/es.(*Factory).CreateArchiveSpanReader(0x107fe3c80?)
	/Users/ysh/dev/jaegertracing/jaeger/plugin/storage/es/factory.go:201 +0x54
github.com/jaegertracing/jaeger/cmd/query/app/querysvc.(*QueryServiceOptions).InitArchiveStorage(0x14000a8ef60, {0x106ad1898?, 0x14000eb6e00}, 0x14000eb8980)
	/Users/ysh/dev/jaegertracing/jaeger/cmd/query/app/querysvc/query_service.go:136 +0x64
github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery.(*server).addArchiveStorage(0x14000ea5c20, 0x14000a8ef60, {0x106ab6d20?, 0x14000a45760?})
	/Users/ysh/dev/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery/server.go:137 +0x98
github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery.(*server).Start(0x14000ea5c20, {0x14000ec6030?, 0x14000f711d0?}, {0x106ab6d20, 0x14000a45760})
	/Users/ysh/dev/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery/server.go:76 +0x204

[mahadzaryab1]

@yurishkuro just wanted to walk through the plan for this refactoring with a couple of questions

1. Should the archive storage now also conform to storage.Factory?

type Factory interface {
	// Initialize performs internal initialization of the factory, such as opening connections to the backend store.
	// It is called after all configuration of the factory itself has been done.
	Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error

	// CreateSpanReader creates a spanstore.Reader.
	CreateSpanReader() (spanstore.Reader, error)

	// CreateSpanWriter creates a spanstore.Writer.
	CreateSpanWriter() (spanstore.Writer, error)

	// CreateDependencyReader creates a dependencystore.Reader.
	CreateDependencyReader() (dependencystore.Reader, error)
}

2. It looks like for each ES storage configuration, we configure both a primary store and an archive store with the same config. As a result, do we need to expose an isArchive flag?

[yurishkuro]

(1) Yes on the first point. I'd say this is more of a question to the binaries using the factory to create readers/writers, right now those binaries cast the Factory to ArchiveFactory, and it will need to change.

(2) It's not from the same config, primary/archive have their own configs (but the latter is derived from the former for defaults, so that people don't need to duplicate CLI flags - we don't plan to support such reuse in v2 config). The isArchive flag only exists in ES storage because it affects how the storage queries the index. We could probably reuse the MaxSpanAge parameter for that.

[mahadzaryab1]

@yurishkuro Following up on (2) - do primary/archive have their own configs in v2 as well? Or is this just in v1?

Another question I had was the jaegerquery extension initializes the ES storage as follows:

		case cfg.Elasticsearch != nil:
			factory, err = es.NewFactoryWithConfig(*cfg.Elasticsearch, mf, s.telset.Logger)

How would it work if we changed the factory to only initialize one config (primary or archive) instead of having both in the same configuration?

[yurishkuro]

do primary/archive have their own configs in v2 as well?

They do. In v2 config we already make no distinction - you declare a storage with some name X of some backend type (e.g. Elastic). Then the query extension has two foreign key fields, PrimaryStore and ArchiveStore. In theory they could point to the same name X (but then the data will be written to the same db), but if they point to different names then you have different configs.

How would it work if we changed the factory to only initialize one config (primary or archive) instead of having both in the same configuration?

The caller is supposed to use different factories for different purposes.

[mahadzaryab1]

do primary/archive have their own configs in v2 as well?

They do. In v2 config we already make no distinction - you declare a storage with some name X of some backend type (e.g. Elastic). Then the query extension has two foreign key fields, PrimaryStore and ArchiveStore. In theory they could point to the same name X (but then the data will be written to the same db), but if they point to different names then you have different configs.

I see. But from the user's perspective in v2 - wouldn't they just define one config and they would get the archive storage out of the box with same configurations?

How would it work if we changed the factory to only initialize one config (primary or archive) instead of having both in the same configuration?

The caller is supposed to use different factories for different purposes.

Got it. But what would the factory get set to in this case? Would the Start function set both the primary and the archive configuration?

[yurishkuro]

factory is not supposed to know if it's used for primary or archive storage, that's the point.

[mahadzaryab1]

@yurishkuro Sorry my question was actually about the extension (https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/internal/extension/jaegerstorage/extension.go#L133). Currently, this part here will initialize both the primary and archive config. If we refactor es.NewFactoryWithConfig to only initialize one storage type, how would the query extension deal with that to ensure both are initialized?

[yurishkuro]

storageextension doesn't care about primary/archive. queryservice does, and it creates to different factories, e.g.

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

Line 132 in b644701

f, err := jaegerstorage.GetStorageFactory(s.config.Storage.TracesArchive, host)

[mahadzaryab1]

@yurishkuro Got it. So when creating the factory using es.NewFactoryWithConfig, how do we specify that the config is primary/archive? Do we have the factory hold an isArchive flag that is passed in through the constructor? You mentioned earlier that it might also be something we expose to the user through the configuration - do we want to take that route and if so, how would that work?

[yurishkuro]

We discussed elsewhere if we even need this flag, or if the same effect can be achieved by tweaking the existing parameters.

[mahadzaryab1]

@yurishkuro I think I'm a bit confused about how the config is laid out. For ES, we have a config like the following:

     some_storage:
        elasticsearch:
          indices:
            index_prefix: "jaeger-main"
            spans:
              date_layout: "2006-01-02"
              rollover_frequency: "day"
              shards: 5
              replicas: 1
            services:
              date_layout: "2006-01-02"
              rollover_frequency: "day"
              shards: 5
              replicas: 1
            dependencies:
              date_layout: "2006-01-02"
              rollover_frequency: "day"
              shards: 5
              replicas: 1
            sampling:
              date_layout: "2006-01-02"
              rollover_frequency: "day"
              shards: 5
              replicas: 1
      another_storage:
        elasticsearch:
          indices:
            index_prefix: "jaeger-archive"

How does each config get "chosen" as the primary or the archive configuration? Is it because of this delegation here?

  jaeger_query:
    storage:
      traces: some_storage
      traces_archive: another_storage

[mahadzaryab1]

We discussed elsewhere if we even need this flag, or if the same effect can be achieved by tweaking the existing parameters.

Yep - that discussion was in #6065 (comment). How would we use maxspanage to decide if we should set isArchive or not?

[mahadzaryab1]

@yurishkuro Another question I had was related to the setup of the config. Currently, the Options config is held in the factory which looks like this. Do we want to remove this struct and have everything be tied to namespaceConfig or even Configuration? I'm not entirely sure if that's possible given that Options is tied to the initialization for v1 storage.

[yurishkuro]

The namespace configs should go away, they were a mechanism for a single factory to represent multiple storages, which is what we're trying to move away from. One factory, one storage namespace, one role (primary/archive).

[yurishkuro]

How would we use maxspanage to decide if we should set isArchive or not?

it's used for both reading and writing, by affecting which index names are utilized. We need to inspect that logic to see if the same selections can be made based on existing parameters.

[mahadzaryab1]

@yurishkuro If we don't want to change the behaviour of the archive storage for v1, then can we still upgrade the v2 storages in place? If we update the storages to the v2 interface in place and get rid of the v1 implementation, we'll lose the CreateArchiveSpanReader and CreateArchiveSpanWriter functions in the process.

[mahadzaryab1]

@yurishkuro Anything else we want to address as part of this issue?

[yurishkuro]

I think we can close, I can't anything high priority.