[ES] Refactor FindTraces and GetTrace of SpanReader to make them reusable for v2 APIs
#6845
Conversation
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
|
Fixing tests! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6845 +/- ##
==========================================
- Coverage 96.06% 96.00% -0.06%
==========================================
Files 367 368 +1
Lines 20787 20839 +52
==========================================
+ Hits 19969 20007 +38
- Misses 624 634 +10
- Partials 194 198 +4
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:
|
| type Trace map[TraceID][]*Span | ||
|
|
||
| // LegacyTraceID is a type extracted from trace id model | ||
| type LegacyTraceID string |
There was a problem hiding this comment.
how is this different from TraceID type?
There was a problem hiding this comment.
This id is used in the query buildTraceByIDQuery. The logic of this getting this id from model.TraceID is this:
func getLegacyTraceID(traceID model.TraceID) dbmodel.LegacyTraceID {
var legacyTraceID string
if traceID.High == 0 {
legacyTraceID = strconv.FormatUint(traceID.Low, 16)
} else {
legacyTraceID = fmt.Sprintf("%x%016x", traceID.High, traceID.Low)
}
return dbmodel.LegacyTraceID(legacyTraceID)
}
Please see that this is not something new, it was already in the codebase and since it is a refactor so I decided not to change it.
There was a problem hiding this comment.
This logic is relevant to v1 reader only. Why does dbmodel and core reader need to know about it?
There was a problem hiding this comment.
This logic is in v1 only but the buildTraceByIDQuery needs legacy trace id, therefore a type is created and accepted as a parameter in FindTraces of core reader: type GetTraceAndLegacyTraceIdFn func(ctx context.Context, traceQuery *TraceQueryParameters) (map[TraceID]LegacyTraceID, error) and called in this way:
func (s *SpanReader) FindTraces(ctx context.Context, traceQuery *dbmodel.TraceQueryParameters, getLegacyIdsFn dbmodel.GetTraceAndLegacyTraceIdFn) (dbmodel.Trace, error) {
ctx, span := s.tracer.Start(ctx, "FindTraces")
defer span.End()
uniqueTraceIDs, err := getLegacyIdsFn(ctx, traceQuery)
if err != nil {
return nil, es.DetailedError(err)
}
return s.multiRead(ctx, uniqueTraceIDs, traceQuery.StartTimeMin, traceQuery.StartTimeMax)
}
There was a problem hiding this comment.
First, given how long ago the "legacy ID" logic was introduced (in 2019), we should introduce a feature gate that by default turns off support for these legacy IDs (allowing users to turn it on for a couple of release). Please make a separate PR for that.
Second, the actual "legacy" logic is simply padding shorter strings with 0s, so such logic (protected by above feature gate) can reside in the core reader without introducing this "legacy ID" duality in the API.
There was a problem hiding this comment.
That means, we have these two things to do:
- Remove the legacy ids and implement feature gate.
- Extract the legacy trace id from
dbmodel.TraceID(string) itself.
Am I understanding right?
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
## Which problem is this PR solving? Fixes a part of #6458 ## Description of the changes - As discussed in the comment #6845 (comment), legacy trace id is moved to feature gate ## How was this change tested? - Unit and Integration tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Manik2708 <[email protected]>
Which problem is this PR solving?
Fixes a part of: #6458
Description of the changes
FindTracesandGetTraceto complete refactoring for ES Span ReaderHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test