refactor(storage): remove v1 downgrades from integration test read paths #7761
Conversation
- Created trace_compare_otlp.go with OTLP helper functions: - CollectOTLPTraces: collects iterator results with error handling - OTLPTracesToV1Slice: converts OTLP traces to v1 slice - GetFirstTrace: extracts first trace with proper Process copying - Modified integration.go to use CollectOTLPTraces in 6 locations: - testGetServices - helperTestGetTrace - testGetTrace (2 places) - findTracesByQuery - Tests still convert to v1 for comparison (will be removed in PR 2) - All 25+ integration tests passing Resolves jaegertracing#7050 (Part 1/2) Signed-off-by: SoumyaRaikwar <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7761 +/- ##
==========================================
+ Coverage 95.63% 95.64% +0.01%
==========================================
Files 310 310
Lines 16051 16051
==========================================
+ Hits 15350 15352 +2
+ Misses 552 551 -1
+ Partials 149 148 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@yurishkuro could you please review my pr |
Removed redundant comments in GetFirstTrace function. Signed-off-by: Soumya Raikwar <[email protected]>
| } | ||
|
|
||
| // CollectOTLPTraces collects all traces from an iterator into a slice | ||
| func CollectOTLPTraces(iterTraces iter.Seq2[[]ptrace.Traces, error]) ([]ptrace.Traces, error) { |
There was a problem hiding this comment.
please use existing functions
internal/storage/v2/v1adapter/translator.go
47:func V1TracesFromSeq2(otelSeq iter.Seq2[[]ptrace.Traces, error]) ([]*model.Trace, er
internal/jptrace/aggregator.go
16:func AggregateTraces(tracesSeq iter.Seq2[[]ptrace.Traces, error]) iter.Seq2[ptrace.Traces, error] {
internal/jiter/iter.go
11:func CollectWithErrors[V any](seq iter.Seq2[V, error]) ([]V, error) {
22:func FlattenWithErrors[V any](seq iter.Seq2[[]V, error]) ([]V, error) {
internal/jptrace/spaniter.go
20:func SpanIter(traces ptrace.Traces) iter.Seq2[SpanIterPos, ptrace.Span] {
There was a problem hiding this comment.
Thanks for the pointers
I’ve updated the storage integration tests to reuse the shared helpers:
jiter.FlattenWithErrorsfor collecting fromiter.Seq2GetFirstTrace/OTLPTracesToV1Slicefor converting to v1 right before comparison
Tests no longer define their own collector, and variable names are back to neutral (traces / actual).
| } | ||
| actual = traces[0] | ||
| return len(actual.Spans) == len(expected.Spans) | ||
| actualOTLP = otlpTraces[0] |
There was a problem hiding this comment.
please avoid unnecessary renaming - once this refactoring is finished we don't need to keep seeing "otlpTraces" when it's just "traces", so there's no need to introduce these names now either
There was a problem hiding this comment.
Renamed back to traces / actual and now only convert to v1 right before comparison. No new otlp* names in the tests anymore.
- Switched to instead of a custom collector - Kept test variable names neutral ( / ) while still converting to v1 for comparison Signed-off-by: SoumyaRaikwar <[email protected]>
|
I’ve updated the storage integration tests to reuse the shared helpers: @yurishkuro could you please review again |
|
@yurishkuro could you please review |
| StartTimeMax: time.Now(), | ||
| }) | ||
| traces, err := v1adapter.V1TracesFromSeq2(iterTraces) | ||
| traces, err := jiter.FlattenWithErrors(iterTraces) |
There was a problem hiding this comment.
what is the point of this change? How is this better than calling v1adapter.V1TracesFromSeq2?
There was a problem hiding this comment.
Sorry for missing that—you're absolutely right. V1TracesFromSeq2 is exactly what we need here; no point in doing the two-step conversion when we already have a helper that does it in one call. Updated.
Signed-off-by: SoumyaRaikwar <[email protected]>
|
@yurishkuro could you please take another look |
| found := s.waitForCondition(t, func(_ *testing.T) bool { | ||
| iterTraces := s.TraceReader.GetTraces(context.Background(), tracestore.GetTraceParams{TraceID: expectedTraceID}) | ||
| traces, err := v1adapter.V1TracesFromSeq2(iterTraces) | ||
| traces, err := jiter.FlattenWithErrors(iterTraces) |
There was a problem hiding this comment.
what is the point of this change? You still transform the result in to v1 model via GetFirstTrace().
2 participants
Which problem is this PR solving?
Description of the changes
trace_compare_otlp.gowith OTLP helper functions:CollectOTLPTraces: collects iterator results with proper error handlingOTLPTracesToV1Slice: converts OTLP traces to v1 slice for comparisonGetFirstTrace: extracts first trace with proper Process deep copying to avoid shared referencesintegration.goto useCollectOTLPTracesinstead ofV1TracesFromSeq2in 6 locations:testGetServices(line 197)helperTestGetTrace(line 238)testGetTracemain logic (line 339)testGetTraceNotFound test (line 362)findTracesByQuery(line 408)How was this change tested?
make lintandmake testsuccessfullySTORAGE=memory go test -v ./internal/storage/integration/ -run TestMemoryStorageChecklist
jaeger:make lint test