[cassandra] Change signatures of CoreSpanReader to return iterators by Manik2708 · Pull Request #8296 · jaegertracing/jaeger

Manik2708 · 2026-04-05T19:34:52Z

Which problem is this PR solving?

Discussed in: [cassandra] Implement v2 reader #8126 (comment)

Description of the changes

To optimize memory, iterators are returned so that we don't need to store traces before conversion

How was this change tested?

Unit Tests

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully: make lint test

AI Usage in this PR (choose one)

See AI Usage Policy.

None: No AI tools were used in creating this PR
Light: AI provided minor assistance (formatting, simple suggestions)
Moderate: AI helped with code generation or debugging specific parts
Heavy: AI generated most or all of the code changes

Copilot

Pull request overview

This pull request modifies the CoreSpanReader interface to return iterators instead of slices, optimizing memory usage by avoiding the need to store traces before conversion. The changes affect the v1 and v2 storage layers, specifically the Cassandra implementation.

Changes:

Changed CoreSpanReader.FindTraces return type from ([]*model.Trace, error) to iter.Seq2[*model.Trace, error] to return an iterator
Changed CoreSpanReader.GetOperations signature to use tracestore.OperationQueryParams (removed the old GetOperationsV2 method)
Updated FindTraceIDs return type to use dbmodel.TraceID instead of model.TraceID
Removed the fallback field from the v2 TraceReader struct and implemented direct iterator-based implementations for GetTraces, FindTraces, and FindTraceIDs
Updated all tests and test utilities to work with the new iterator-based interfaces

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/storage/v1/cassandra/spanstore/reader.go	Updated CoreSpanReader interface, implemented FindTraces to return iter.Seq2, changed FindTraceIDs return type to dbmodel.TraceID, consolidated GetOperations method
internal/storage/v2/cassandra/tracestore/reader.go	Removed fallback field, implemented iterator-based GetTraces, FindTraces, and FindTraceIDs methods directly instead of delegating to adapter
internal/storage/v1/cassandra/spanstore/reader_test.go	Updated FindTraces tests to consume the iterator pattern
internal/storage/v2/cassandra/tracestore/reader_test.go	Added comprehensive tests for new iterator-based methods
internal/storage/v1/cassandra/spanstore/mocks/mocks.go	Updated mock CoreSpanReader to reflect new interface signatures
internal/storage/v1/cassandra/savetracetest/main.go	Updated queryAndPrint function to consume FindTraces iterator

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Manik2708 · 2026-04-05T19:39:14Z

@yurishkuro I think workflows require approval from maintainers. Can you run the workflows?

codecov · 2026-04-05T20:01:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.60%. Comparing base (957af97) to head (35bb49c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8296      +/-   ##
==========================================
- Coverage   95.63%   95.60%   -0.03%     
==========================================
  Files         314      314              
  Lines       16505    16505              
==========================================
- Hits        15784    15780       -4     
- Misses        568      571       +3     
- Partials      153      154       +1

Flag	Coverage Δ
badger_direct	`9.31% <0.00%> (ø)`
badger_e2e	`1.07% <0.00%> (ø)`
cassandra-4.x-direct-manual	`13.60% <33.33%> (-0.04%)`	⬇️
cassandra-4.x-e2e-auto	`1.06% <0.00%> (ø)`
cassandra-4.x-e2e-manual	`1.06% <0.00%> (ø)`
cassandra-5.x-direct-manual	`13.60% <33.33%> (-0.04%)`	⬇️
cassandra-5.x-e2e-auto	`1.06% <0.00%> (ø)`
cassandra-5.x-e2e-manual	`1.06% <0.00%> (ø)`
clickhouse	`1.20% <0.00%> (ø)`
elasticsearch-6.x-direct	`17.49% <0.00%> (ø)`
elasticsearch-7.x-direct	`17.53% <0.00%> (ø)`
elasticsearch-8.x-direct	`17.68% <0.00%> (ø)`
elasticsearch-8.x-e2e	`1.07% <0.00%> (ø)`
elasticsearch-9.x-e2e	`1.07% <0.00%> (ø)`
grpc_direct	`8.09% <0.00%> (ø)`
grpc_e2e	`1.07% <0.00%> (ø)`
kafka-3.x-v2	`1.07% <0.00%> (ø)`
memory_v2	`1.07% <0.00%> (ø)`
opensearch-1.x-direct	`17.57% <0.00%> (ø)`
opensearch-2.x-direct	`17.57% <0.00%> (ø)`
opensearch-2.x-e2e	`1.07% <0.00%> (ø)`
opensearch-3.x-e2e	`1.07% <0.00%> (ø)`
query	`1.07% <0.00%> (ø)`
tailsampling-processor	`0.54% <0.00%> (ø)`
unittests	`94.25% <100.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mahadzaryab1

just a couple of questions - lets try to make incremental changes if possible to make the changes easier to review

internal/storage/v1/cassandra/spanstore/reader.go

internal/storage/v2/cassandra/tracestore/reader.go

Manik2708 · 2026-04-06T17:59:04Z

@mahadzaryab1 i have trimmed the scope of this PR by changing just FindTraces. if we want to change the signatures of rest of the methods too then we can work on them

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-04-06T18:22:24Z

CI Summary Report

Metrics Comparison

✅ No significant metric changes

Code Coverage

✅ Coverage 96.8% (baseline 96.8%)

➡️ View CI run | View publish logs
2026-04-09 21:23:08 UTC

internal/storage/v1/cassandra/spanstore/reader.go

Manik2708 · 2026-04-08T19:42:52Z

@yurishkuro I have implemented the adapter as per my understanding if this looks fine i will start working on covering the edge cases for code coverage

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Manik2708 · 2026-04-08T19:46:44Z

@yurishkuro adapter seemed a bridge to me just like v1 adapter so I transferred v2 functions to CoreSpanReader. If we want opposite i can make changes

internal/storage/v1/cassandra/spanstore/reader.go

Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>

internal/storage/v1/cassandra/spanstore/reader.go

yurishkuro · 2026-04-09T20:23:24Z

internal/storage/v1/cassandra/spanstore/reader_test.go

+		for _, err := range r.reader.FindTraces(context.Background(), nil) {
+			require.Empty(t, r.traceBuffer.GetSpans(), "Spans Not recorded")
+			require.Error(t, err)
+		}


not equivalent, the loop may simply exit and you're not going to execute any require* steps.

Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>

Manik2708 · 2026-04-09T20:57:39Z

internal/storage/v1/cassandra/spanstore/reader_test.go

+		for _, err := range r.reader.FindTraces(context.Background(), nil) {
+			require.Error(t, err)
+		}
 		require.Empty(t, r.traceBuffer.GetSpans(), "Spans Not recorded")


i think now it will be fine

Manik2708 requested a review from a team as a code owner April 5, 2026 19:34

Copilot AI review requested due to automatic review settings April 5, 2026 19:34

dosubot bot added area/storage enhancement storage/cassandra labels Apr 5, 2026

Copilot started reviewing on behalf of Manik2708 April 5, 2026 19:35 View session

Copilot AI reviewed Apr 5, 2026

View reviewed changes

mahadzaryab1 reviewed Apr 5, 2026

View reviewed changes

internal/storage/v1/cassandra/spanstore/reader.go Outdated Show resolved Hide resolved

internal/storage/v1/cassandra/spanstore/reader.go Outdated Show resolved Hide resolved

internal/storage/v2/cassandra/tracestore/reader.go Outdated Show resolved Hide resolved

Manik2708 marked this pull request as draft April 5, 2026 20:25

Manik2708 force-pushed the v1-reader branch from 2ef7c12 to f454171 Compare April 6, 2026 17:57

Manik2708 marked this pull request as ready for review April 6, 2026 17:59

Manik2708 requested review from Copilot and mahadzaryab1 April 6, 2026 17:59

Copilot started reviewing on behalf of Manik2708 April 6, 2026 17:59 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

mahadzaryab1 reviewed Apr 7, 2026

View reviewed changes

internal/storage/v1/cassandra/spanstore/reader.go Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings April 8, 2026 19:41

Manik2708 requested review from mahadzaryab1 and yurishkuro April 8, 2026 19:43

Copilot started reviewing on behalf of Manik2708 April 8, 2026 19:43 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

yurishkuro reviewed Apr 8, 2026

View reviewed changes

internal/storage/v1/cassandra/spanstore/reader.go Outdated Show resolved Hide resolved

yurishkuro reviewed Apr 8, 2026

View reviewed changes

internal/storage/v1/cassandra/spanstore/reader.go Outdated Show resolved Hide resolved

conflicts

af58a2d

Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>

Manik2708 force-pushed the v1-reader branch from 3a72a85 to af58a2d Compare April 9, 2026 19:56

Merge branch 'main' into v1-reader

2edd199

Manik2708 commented Apr 9, 2026

View reviewed changes

internal/storage/v1/cassandra/spanstore/reader.go Show resolved Hide resolved

Manik2708 requested review from Copilot and yurishkuro April 9, 2026 19:59

Copilot started reviewing on behalf of Manik2708 April 9, 2026 20:00 View session

yurishkuro reviewed Apr 9, 2026

View reviewed changes

fix

624b49a

Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>

Manik2708 commented Apr 9, 2026

View reviewed changes

Manik2708 requested a review from yurishkuro April 9, 2026 20:57

Merge branch 'main' into v1-reader

35bb49c

Conversation

Manik2708 commented Apr 5, 2026

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

AI Usage in this PR (choose one)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Manik2708 commented Apr 5, 2026

Uh oh!

codecov bot commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mahadzaryab1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Manik2708 commented Apr 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions bot commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Summary Report

Metrics Comparison

Code Coverage

Uh oh!

Uh oh!

Manik2708 commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Manik2708 commented Apr 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yurishkuro Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Manik2708 Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Apr 5, 2026 •

edited

Loading

mahadzaryab1 left a comment •

edited

Loading

github-actions bot commented Apr 6, 2026 •

edited

Loading