[WIP] Upgrade storage integration test: Use V2 Archive ReaderWriter #6489
Conversation
ekefan
force-pushed
the
archive-reader-writer
branch
from
2ca2f23 to
9ad6870
Compare
yurishkuro
added
the
changelog:ci
|
looks like some archive tests are failing |
|
there's no new change with the recent push... just trying to update the branch. I will get back to these issues later in my day. |
ekefan
force-pushed
the
archive-reader-writer
branch
from
aaab620 to
a43bc9a
Compare
|
Hello @yurishkuro, I can't find the cause of this bug, but for elastic search and open search, the testArchiveTrace test fails; the archive trace reader returns an empty trace... |
|
Did you look at the logs? I see nil pointer segfaults, that should be easy to troubleshoot. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6489 +/- ##
==========================================
- Coverage 96.24% 96.22% -0.03%
==========================================
Files 373 373
Lines 21406 21409 +3
==========================================
- Hits 20602 20600 -2
- Misses 612 616 +4
- Partials 192 193 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ekefan
force-pushed
the
archive-reader-writer
branch
from
ccf0435 to
54a0a07
Compare
ekefan
changed the title
ekefan
force-pushed
the
archive-reader-writer
branch
from
1febf80 to
c678a64
Compare
ekefan
force-pushed
the
archive-reader-writer
branch
from
7e58074 to
cc0a508
Compare
ekefan
force-pushed
the
archive-reader-writer
branch
from
cc0a508 to
7486084
Compare
| @@ -65,6 +65,11 @@ func (tr *TraceReader) GetTraces( | |||
| } | |||
| } | |||
|
|
|||
| func (tr *TraceReader) GetArchiveTraces(ctx context.Context, traceID tracestore.GetTraceParams) (*model.Trace, error) { | |||
There was a problem hiding this comment.
Hello @yurishkuro,
For some reason, getting archive traces on es v1 fails when looping through traceIDs
I spent some time, reading codes in the es dir, and recent changes to config for archive traces but I couldn't find anything that pointed the reason for not reading traces. Then I tried this GetArchiveTraces function:
func (tr *TraceReader) GetArchiveTraces(ctx context.Context, traceIDs ...tracestore.GetTraceParams) ([]*model.Trace, error) {
traces := []*model.Trace{}
var err error
for _, traceID := range traceIDs {
tID := model.TraceIDFromOTEL(traceID.TraceID)
trace, err := tr.spanReader.GetTrace(ctx, spanstore.GetTraceParameters{TraceID: tID})
if err != nil {
if errors.Is(err, spanstore.ErrTraceNotFound) {
fmt.Printf("Trace with id: %v, not found", tID)
continue
} else {
return nil, err
}
}
traces = append(traces, trace)
}
return traces, err
}and it failed to read too..
But this variant didn't fail..
func (tr *TraceReader) GetArchiveTraces(ctx context.Context, traceIDs ...tracestore.GetTraceParams) ([]*model.Trace, error) {
traces := []*model.Trace{}
traceID := traceIDs[0] // trying this out, since only one archive trace is to be read
tID := model.TraceIDFromOTEL(traceID.TraceID)
trace, err := tr.spanReader.GetTrace(ctx, spanstore.GetTraceParameters{TraceID: tID})
if err != nil {
if errors.Is(err, spanstore.ErrTraceNotFound) {
fmt.Printf("Trace with id: %v, not found", tID)
}
return nil, err
}
traces = append(traces, trace)
return traces, err
}Since, all jaeger backend storage will be upgraded to version 2, we could use this function till then.
What do you think? about the bug, the suggestion?
ekefan
force-pushed
the
archive-reader-writer
branch
2 times, most recently
from
fb698f7 to
2d8a157
Compare
| // if len(traces) > 0 { | ||
| // actual = traces[0] | ||
| // } | ||
| actual, err = s.ArchiveTraceReader.(*v1adapter.TraceReader).GetArchiveTraces(context.Background(), tracestore.GetTraceParams{TraceID: tID.ToOTELTraceID()}) |
There was a problem hiding this comment.
we should not assume that the ArchiveTraceReader is implemented as v1adapter. You can use v2 API to read ptrace.Traces, then convert that into model.Trace for comparison.
| @@ -65,6 +65,11 @@ func (tr *TraceReader) GetTraces( | |||
| } | |||
| } | |||
|
|
|||
| func (tr *TraceReader) GetArchiveTraces(ctx context.Context, traceID tracestore.GetTraceParams) (*model.Trace, error) { | |||
There was a problem hiding this comment.
there is no such thing as GetArchiveTraces, we never had it even in v1 storage interface, and we explicitly are moving away from in it v2.
There was a problem hiding this comment.
I totally get that...
I only created it for the purpose of solving the bug we are experiencing hoping it would be no longer be needed soon.
Signed-off-by: Emmanuel Emonueje Ebenezer <eebenezer949@gmail.com>
Signed-off-by: Emmanuel Emonueje Ebenezer <eebenezer949@gmail.com>
ekefan
force-pushed
the
archive-reader-writer
branch
from
2d8a157 to
120f2f1
Compare
|
@yurishkuro, @mahadzaryab1, I have two questions please:
|
|
I'm currently in the process of trying to completely phase out the difference between primary and archive interfaces in #6567. If that can be pulled off, then this PR can initialize primary and archive storage in the same way. |
Which problem is this PR solving?
Description of the changes
Incrementally swaps the fields of
StorageIntegrationto align with v2 storage apiUpdates test functions accordingly to work with the updated fields
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test