-
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
[WIP] Refactor storage factories to hold one configuration #6156
base: main
Are you sure you want to change the base?
[WIP] Refactor storage factories to hold one configuration #6156
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
63d9d38
to
209b834
Compare
plugin/storage/factory.go
Outdated
@@ -118,7 +118,7 @@ func (*Factory) getFactoryOfType(factoryType string) (storage.Factory, error) { | |||
case cassandraStorageType: | |||
return cassandra.NewFactory(), nil | |||
case elasticsearchStorageType, opensearchStorageType: | |||
return es.NewFactory(), nil | |||
return es.NewFactory(es.PrimaryNamespace), nil |
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 can you take another look at the changes? I minimized the code movements to simply remove the others
field from Options
. The problem now is that this isn't initializing the CLI flags for es-archive
. Any thoughts on how to get around this?
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.
see #6156 (comment)
The query service instantiates just a single factory and casts it to ArchiveFactory. With your changes (which are in the right direction, but of insufficient scope) it never gets a chance to create archive CLI flags, because that has to happen via different factories.
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 is the archive factory only needed for the query service?
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
) | ||
|
||
var ( // interface comformance checks | ||
_ storage.Factory = (*Factory)(nil) | ||
_ storage.ArchiveFactory = (*Factory)(nil) |
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.
need to be very careful about removing ArchiveFactory interface, because query service uses it via runtime cast, so unless we have integration tests (many of them disable archive tests as I recall) you can introduce a breaking change
archiveFactory, ok := storageFactory.(storage.ArchiveFactory) |
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 yep - noted. I was thinking to avoid breaking changes we can remove the ArchiveFactory within this PR and use an archive storage factory wherever needed.
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.
https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/integration/elasticsearch_test.go#L165 - ES doesn't skip the archive test
Signed-off-by: Mahad Zaryab <[email protected]>
@yurishkuro we've got 3 callsites for
|
I don't think you need to change (1), but we need to change InitArchiveStorage() not to cast but to use the storage directly (2) yes (3) I think you don't need to change it because if the caller needs an archive storage it should instantiate a different remote storage. As I understand it there's not a dedicated gRPC API for archive storage, which could go away the same way as ArchiveFactory. |
@yurishkuro Got it. For (2) - this is how the primary storage factory is initialized. How would we go about initializing the archive storage factory here to expose the CLI flags for storages that have archive flags (cassandra/es) |
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
@yurishkuro For v1, what do you think of passing the |
6789a49
to
a4d41b9
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
// TODO: what should we do here? | ||
// _ = qOpts.InitArchiveStorage(f, logger) |
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 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.
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.
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.
Signed-off-by: Mahad Zaryab <[email protected]>
cefc464
to
bbcdbd7
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6156 +/- ##
==========================================
- Coverage 96.47% 96.44% -0.04%
==========================================
Files 354 354
Lines 20126 19997 -129
==========================================
- Hits 19417 19286 -131
- Misses 524 526 +2
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
@yurishkuro For ES Archive, it looks like the archive flag is used in two places:
For Cassandra, its a bit more straightforward:
Do you have any thoughts on how we should proceed? Do we still want to expose an |
The metrics namespacing for Cassandra can be easily done elsewhere, does not need to be based on isArchive. It's only needed there now because primary/archive distinction is made internally in the factory. For ES:
|
With the setup of v2, how would we make that distinction?
@yurishkuro Okay I see. But if the configurations are being held in different factories - how would we perform validation there?
I was thinking the same as well. I'm guessing its an optimization to avoid sorting the archive storage which would be larger than the a non-archive storage. Here is the documentation for Search After. Would we have a performance degradation here if we enabled this for archive as well?
Ah yes. I was only looking at the reader. Are you referring to |
the storage extension manages factories and knows storage names, it can use those names to bound MetricsFactory to have a specific label.
No, configuration are passed to factories, but held in a single place in storage extension, which can invoke additional validation here:
but the thing is, we never search for traces in archive storage, we only retrieve trace by ID, so sorting in this case would only apply to the spans within a trace - yes, could have overhead for very large trace, but still small. |
not just suffix, when it's primary storage with manually rotated indices they also have the date pattern in the name, but archive index never has that (because it doesn't grow large). One compromise we could do is recommend that users don't use archive storage with manually rotated indices, only with ILM. Unless there's another way that I am not seeing. Btw, reader also has similar branching in index naming logic: jaeger/plugin/storage/es/spanstore/reader.go Line 174 in a420fd9
|
So this is where the Cassandra storage is initialized in the extension. Are you suggesting we can pass the label into the constructor here? If so, how would we make the distinction based on just the name?
Oh okay, I see. So whenever we're processing an ES config, we would go through all the other ones that exist and make sure that the index prefixes are not the same?
Sounds good. I can remove the indirection here then. |
How would making that recommendation simplify the archive branching for us? |
ecc679f
to
7f87200
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
8fccbac
to
7d65eb8
Compare
@yurishkuro Regarding |
272749b
to
655a666
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
655a666
to
c385416
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
@@ -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:"-"` |
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.
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.
} | ||
|
||
// NewFactory creates a new Factory. | ||
func NewFactory() *Factory { | ||
func NewFactory(isArchive bool) *Factory { |
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.
nit: let's not pass a flag but have a 2nd constructor NewArchiveFactory. Because NewFactory(true)
has poor readability.
@@ -49,9 +49,10 @@ const ( | |||
// (e.g. archive) may be underspecified and infer the rest of its parameters from primary. | |||
type Options struct { | |||
Primary NamespaceConfig `mapstructure:",squash"` |
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.
Embed?
Primary NamespaceConfig `mapstructure:",squash"` | |
NamespaceConfig `mapstructure:",squash"` |
@@ -123,96 +118,55 @@ func (f *Factory) InitFromViper(v *viper.Viper, _ *zap.Logger) { | |||
// configureFromOptions configures factory from Options struct. | |||
func (f *Factory) configureFromOptions(o *Options) { | |||
f.Options = o | |||
f.primaryConfig = f.Options.GetPrimary() | |||
f.archiveConfig = f.Options.Get(archiveNamespace) | |||
f.config = f.Options.GetPrimary() |
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.
GetPrimary
is now meaningless, can we do something else?
Directionally this LGTM. Any blockers? |
@yurishkuro The main blocker is that currently this implementation doesn't initialize the CLI flags for archive storage. We initialize the storage factory in https://github.com/jaegertracing/jaeger/blob/main/cmd/all-in-one/main.go#L58 which in this PR will only initialize the primary storages because the storage factories only hold one configuration. Any thoughts on how we should initialize the archive CLI flags? |
v1 |
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test