[cassandra][v2] Refactor FromDbModel and ToDbModel to accept and return dbmodel.Span
#7844
Conversation
|
Mistakenly closed the last one, please review @yurishkuro |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7844 +/- ##
==========================================
+ Coverage 95.45% 95.67% +0.22%
==========================================
Files 301 301
Lines 15855 15762 -93
==========================================
- Hits 15134 15080 -54
+ Misses 565 541 -24
+ Partials 156 141 -15
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:
|
Metrics Comparison SummaryTotal changes across all snapshots: 0 Detailed changes per snapshotsummary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
summary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the Cassandra v2 trace store converters to work directly with dbmodel.Span instead of Jaeger protobuf models. The changes eliminate intermediate conversions and simplify the data flow by having FromDbModel and ToDbModel functions accept and return dbmodel.Span types.
Changes:
- Refactored converter functions to use
dbmodel.Spandirectly instead ofmodel.Batch - Updated type constants in dbmodel from private to public (e.g.,
stringType→StringType) - Removed intermediate batch grouping logic and process checksumming functionality
- Updated test fixtures to reflect the new data structure
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/storage/v2/cassandra/tracestore/to_dbmodel.go | Refactored conversion from ptrace.Traces to dbmodel.Span with direct field mappings |
| internal/storage/v2/cassandra/tracestore/to_dbmodel_test.go | Updated tests to use dbmodel types and added edge case coverage |
| internal/storage/v2/cassandra/tracestore/from_dbmodel.go | Simplified conversion from dbmodel.Span to ptrace.Traces, removed batch regrouping logic |
| internal/storage/v2/cassandra/tracestore/from_dbmodel_test.go | Updated tests to work with dbmodel.Span instead of model.Batch |
| internal/storage/v1/cassandra/spanstore/dbmodel/model.go | Exported type constants for public API usage |
| internal/storage/v1/cassandra/spanstore/dbmodel/converter.go | Updated references to use exported constant names |
| internal/storage/v2/cassandra/tracestore/fixtures/cas_01.json | Updated fixture to reflect dbmodel.Span structure |
| internal/storage/v2/cassandra/tracestore/fixtures/otel_traces_01.json | Added missing flags and status fields to test data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //nolint:gosec // G115 // dbspan.SpanID is guaranteed non-negative by schema constraints | ||
| span.SetSpanID(idutils.UInt64ToSpanID(uint64(dbspan.SpanID))) |
There was a problem hiding this comment.
The nolint directive on line 68 states that dbspan.SpanID is guaranteed non-negative by schema constraints, but int64 can be negative. Consider changing the dbmodel.Span.SpanID field type to uint64 or int64 with validation to ensure this constraint is enforced at the type level rather than relying on comments.
| //nolint:gosec // G115 // dbspan.SpanID is guaranteed non-negative by schema constraints | |
| span.SetSpanID(idutils.UInt64ToSpanID(uint64(dbspan.SpanID))) | |
| if dbspan.SpanID >= 0 { | |
| span.SetSpanID(idutils.UInt64ToSpanID(uint64(dbspan.SpanID))) | |
| } |
There was a problem hiding this comment.
That leads to schema change and we don't know what need to be covered for backward compatibility. Doing this can be a different PR
| //nolint:gosec // G115 // span.StartTime is guaranteed non-negative by schema constraints | ||
| StartTime: int64(span.StartTimestamp()), | ||
| //nolint:gosec // G115 // span.EndTime - span.StartTime is guaranteed non-negative by schema constraints | ||
| Duration: int64(model.DurationAsMicroseconds(span.EndTimestamp().AsTime().Sub(startTime))), |
There was a problem hiding this comment.
The duration calculation converts to microseconds but the variable is named Duration without units, making the unit ambiguous. The comment indicates microseconds, but explicit field documentation or a more descriptive name would improve clarity.
There was a problem hiding this comment.
what does this mean? where do we need to add documentation?
1 participant
Which problem is this PR solving?
Description of the changes
ptrace.Traceto database models of cassandraHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test