[storage] Remove dependency on archive flag in ES reader#6490
[storage] Remove dependency on archive flag in ES reader#6490mahadzaryab1 merged 33 commits intojaegertracing:mainfrom
Conversation
|
@yurishkuro How should we handle the archive namespacing in the reader and factory as they are currently hardcoded. |
mahadzaryab1
changed the title
mahadzaryab1
force-pushed
the
es-archive-dependency
branch
from
20e7ccc to
944f4e0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6490 +/- ##
==========================================
+ Coverage 96.21% 96.23% +0.02%
==========================================
Files 372 372
Lines 21360 21363 +3
==========================================
+ Hits 20551 20559 +8
+ Misses 616 612 -4
+ Partials 193 192 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mahadzaryab1
force-pushed
the
es-archive-dependency
branch
2 times, most recently
from
712c974 to
9adf03e
Compare
mahadzaryab1
force-pushed
the
es-archive-dependency
branch
from
67145ac to
2f7a9be
Compare
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1
force-pushed
the
es-archive-dependency
branch
from
2f7a9be to
0ae679d
Compare
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
plugin/storage/es/factory.go
Outdated
| suffix := "archive" | ||
| if f.archiveConfig.UseReadWriteAliases { | ||
| suffix += "-read" | ||
| } | ||
| sr, err := createSpanReader(f.getArchiveClient, f.archiveConfig, f.logger, f.tracer, suffix, true) |
There was a problem hiding this comment.
@yurishkuro Not ideal to have to do this but this is roughly what we need to maintain backwards compatibility.
There was a problem hiding this comment.
I'm not sure if we want to expose a configuration option to override the default read and write alias but that could be one way to streamline this.
There was a problem hiding this comment.
- who adds read/write suffix to the index name?
- how does that work for non-archive, does it not use
-readsuffix?
There was a problem hiding this comment.
who adds read/write suffix to the index name?
Its added by the reader and the writer but we need to handle the special case where the archive storage also has use_aliases set to true
how does that work for non-archive, does it not use -read suffix?
Yeah it does - its done by the reader and the writer. I made some changes to make it an else-if change. The current behavior is that if a suffix is passed in, we use that. If a suffix isn't passed in but use_aliases is set, then we use the default read/write aliases.
There was a problem hiding this comment.
that's why I don't follow - if reader/writer automatically add -read/-write suffix, why do you need to add it manually above?
There was a problem hiding this comment.
I add it above just to address the case where use_aliases wasn't initially set for the archive storage. Since we hardcode it to true for the archive storage now, we'll always set the index to be archive-read and archive-write in the reader/writer. But that would break backwards compatibility because previously it would just be archive for both the read and write alias for the archive storage.
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
plugin/storage/es/factory_test.go
Outdated
| f := NewFactory() | ||
| f.primaryConfig = &escfg.Configuration{} | ||
| f.archiveConfig = &escfg.Configuration{Enabled: true} | ||
| f.archiveConfig = &escfg.Configuration{Enabled: true, UseReadWriteAliases: true} |
There was a problem hiding this comment.
is this change necessary? It goes contrary to default config.
There was a problem hiding this comment.
@yurishkuro This was mostly for code coverage purposes. I can remove it but then we may need to merge with less than 100% code coverage or add a new test that goes through that code path
There was a problem hiding this comment.
I think my comment on the archive tests would address cove coverage
There was a problem hiding this comment.
@yurishkuro It didn't because that test doesn't go through CreateArchiveSpanReader. I just added a test for both cases here.
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
|
|
||
| func TestSpanReader_ArchiveTraces(t *testing.T) { | ||
| withArchiveSpanReader(t, false, func(r *spanReaderTest) { | ||
| withArchiveSpanReader(t, true, "archive", func(r *spanReaderTest) { |
There was a problem hiding this comment.
by passing the suffix explicitly (which no production code does today) you're overriding the default behavior. It's ok to do this as one of the tests, but we should keep the "default" tests where only use_aliases flag is varied and check that it affects the index name. And then for passing a suffix explicitly like this I would use a completely different string like foobar, to ensure that the validation is not mistaking the index name from default behavior with the index name with overwritten suffix.
Perhaps it makes sense to combine into a table driven test
| use_aliases | input suffix | expected index |
|---|---|---|
| false | "" | ... |
| true | "" | ... |
| false | "foobar" | ... |
| true | "foobar" | ... |
There was a problem hiding this comment.
@yurishkuro Done. Can we remove the test below this now?
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
plugin/storage/es/factory.go
Outdated
| Archive: archive, | ||
| UseReadWriteAliases: cfg.UseReadWriteAliases, | ||
| UseReadWriteAliases: useReadWriteAliases, | ||
| WriteAlias: writeAliasSuffix, |
There was a problem hiding this comment.
| WriteAlias: writeAliasSuffix, | |
| WriteAliasSuffix: writeAliasSuffix, |
| }{ | ||
| {false, "", "jaeger-span-"}, | ||
| {true, "", "jaeger-span-read"}, | ||
| {false, "archive", "jaeger-span-"}, |
There was a problem hiding this comment.
| {false, "archive", "jaeger-span-"}, | |
| {false, "foobar", "jaeger-span-"}, |
Is "jaeger-span-" correct?
There was a problem hiding this comment.
doesn't seem like it would match previous behavior
There was a problem hiding this comment.
@yurishkuro I ran an equivalent test on main
func TestSpanReader_ArchiveTraces(t *testing.T) {
testCases := []struct {
useAliases bool
archive bool
expected string
}{
{false, false, "jaeger-span-"},
{true, false, "jaeger-span-read"},
{false, true, "jaeger-span-archive"},
{true, true, "jaeger-span-archive-read"},
}
for _, tc := range testCases {
t.Run(fmt.Sprintf("useAliases=%v archive=%v", tc.useAliases, tc.archive), func(t *testing.T) {
withArchiveSpanReader(t, tc.useAliases, tc.archive, func(r *spanReaderTest) {
mockSearchService(r).
Return(&elastic.SearchResult{}, nil)
mockArchiveMultiSearchService(r, tc.expected).
Return(&elastic.MultiSearchResult{
Responses: []*elastic.SearchResult{},
}, nil)
query := spanstore.GetTraceParameters{}
trace, err := r.reader.GetTrace(context.Background(), query)
require.NotEmpty(t, r.traceBuffer.GetSpans(), "Spans recorded")
require.Nil(t, trace)
require.EqualError(t, err, "trace not found")
})
})
}
}Case 1 and 2 are the same.
There is a slight difference for case 3 and 4 because we had to change the reader to accommodate some edge cases - with the most important one being the case where archive storage also has read_aliases set to true. However, we moved that check to the factory. So while the behaviour of the reader is slightly different, the overall behaviour should be the same which we see from the integration tests passing.
// CreateArchiveSpanReader implements storage.ArchiveFactory
func (f *Factory) CreateArchiveSpanReader() (spanstore.Reader, error) {
if !f.archiveConfig.Enabled {
return nil, nil
}
readAliasSuffix := "archive"
if f.archiveConfig.UseReadWriteAliases {
readAliasSuffix += "-read"
}
sr, err := createSpanReader(f.getArchiveClient, f.archiveConfig, f.logger, f.tracer, readAliasSuffix, true)
if err != nil {
return nil, err
}
archiveMetricsFactory := f.metricsFactory.Namespace(
metrics.NSOptions{
Tags: map[string]string{
"role": "archive",
},
},
)
return spanstoremetrics.NewReaderDecorator(sr, archiveMetricsFactory), nil
}Case 3 isn't possible anymore because we can never have a case where the readAliasSuffix is set but use_aliases isn't (we hardcode use_aliases to true in the above). And for case 4, the differentiation between archive and archive-read is now just handled by CreateArchiveSpanReader instead of inside the reader.
There was a problem hiding this comment.
For v2, the user can get the archive behaviour by just setting use_aliases to true. It is a breaking change because the storage won't be reading/writing to and from the *-archive index anymore. But I believe that's something that we said we're okay with according to #6519
There was a problem hiding this comment.
v2 callers are still using create-archive functions. Does it mean they will be getting the same index names as before this change without any changes to the config?
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
| }{ | ||
| {false, "", "jaeger-span-"}, | ||
| {true, "", "jaeger-span-read"}, | ||
| {false, "foobar", "jaeger-span-"}, |
There was a problem hiding this comment.
this is the only one that I don't understand, why is it getting a weird index name?
but it's not a blocker since passing the suffix is new functionality, so not a braking change (but possibly a bug).
There was a problem hiding this comment.
@yurishkuro This is from this block. I made it so that we only look at the readAliasSuffix if use_aliases is true. We could change it to always be considered but I thought that might be confusing because then there would be two paths to set an alias rather than one. Let me know what you think. It shouldn't make a difference either way because use_aliases is always set to true for the archive storage while also passing in a custom readAliasSuffix.
readAliasSuffix := ""
// Setting the maxSpanAge to a large duration will ensure all spans in the "read" alias are accessible by queries (query window = [now - maxSpanAge, now]).
// When read/write aliases are enabled, which are required for index rollovers, only the "read" alias is queried and therefore should not affect performance.
if p.UseReadWriteAliases {
maxSpanAge = dawnOfTimeSpanAge
if p.ReadAliasSuffix != "" {
readAliasSuffix = p.ReadAliasSuffix
} else {
readAliasSuffix = "read"
}
}There was a problem hiding this comment.
this code would never result in "jaeger-span-" though, right? It would either produce "jaeger-span-read" (the else clause) or "jaeger-span-${ReadAliasSuffix}"
There was a problem hiding this comment.
yeah exactly - that's tested in TestSpanReaderIndices
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
only one remaining question, but otherwise I feel like we spend enough energy trying to ensure backwards compatibility.
2 participants
Which problem is this PR solving?
Description of the changes
use_aliasesconfiguration so thearchivebool in the archive reader was mostly replaced by theuse_aliasesconfiguration (more details on this are in the description of Phase out the distinction between primary and archive storage #6065).CreateArchiveSpanReaderandCreateArchiveSpanWriterHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test