[ES] Refactored a part of SpanReader to make it reusable for V2 APIs #6824
Conversation
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
This reverts commit 2760cdc. Signed-off-by: Manik2708 <[email protected]>
…PIs" This reverts commit 1fb531d. Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6824 +/- ##
==========================================
- Coverage 96.06% 96.00% -0.07%
==========================================
Files 364 366 +2
Lines 20712 20819 +107
==========================================
+ Hits 19897 19987 +90
- Misses 622 634 +12
- Partials 193 198 +5
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:
|
|
@mahadzaryab1 @yurishkuro This PR tries to make Or we have to re-implement the multiread method by making some query methods as public (which will be used in the new multiread) |
|
@Manik2708 the DB layer treats IDs as strings, so the underlying reader can accept them as strings and make it the responsibility of v1/v2 readers to do a conversion |
Signed-off-by: Manik2708 <[email protected]>
| multiSearchService := &mocks.MultiSearchService{} | ||
| firstMultiSearch := &mocks.MultiSearchService{} | ||
| secondMultiSearch := &mocks.MultiSearchService{} | ||
| multiSearchService.On("Add", id1Search, id2Search).Return(firstMultiSearch) | ||
| multiSearchService.On("Add", id1SearchSpanTime).Return(secondMultiSearch) | ||
| multiSearchService.On("Add", mock.AnythingOfType("*elastic.SearchRequest"), mock.AnythingOfType("*elastic.SearchRequest")).Return(firstMultiSearch) |
There was a problem hiding this comment.
The reason to do this is because currently it is giving the weired unequal address error:
Add(elastic.SearchRequest) 0: &elastic.SearchRequest{searchType:"", indices:[]string(nil), types:[]string(nil), routing:(string)(nil), preference:(string)(nil), requestCache:(bool)(nil), allowPartialSearchResults:(bool)(nil), ignoreUnavailable:(bool)(0xc00032d320), allowNoIndices:(bool)(nil), expandWildcards:"", scroll:"", source:(elastic.SearchSource)(0xc000349040), searchSource:(elastic.SearchSource)(0xc0003491e0), batchedReduceSize:(int)(nil), maxConcurrentShardRequests:(int)(nil), preFilterShardSize:(int)(nil)}
The closest call I have is:
Add(elastic.SearchRequest) 0: &elastic.SearchRequest{searchType:"", indices:[]string(nil), types:[]string(nil), routing:(string)(nil), preference:(string)(nil), requestCache:(bool)(nil), allowPartialSearchResults:(bool)(nil), ignoreUnavailable:(bool)(0xc00032cf30), allowNoIndices:(bool)(nil), expandWildcards:"", scroll:"", source:(elastic.SearchSource)(0xc000348820), searchSource:(elastic.SearchSource)(0xc000348680), batchedReduceSize:(int)(nil), maxConcurrentShardRequests:(int)(nil), preFilterShardSize:(int)(nil)}
There was a problem hiding this comment.
how did it work before? what changed?
There was a problem hiding this comment.
Earlier the On event was getting triggered on specific parameters but when I changed the parameter of buildTraceByIDQuery from model.TraceID to dbmodel.TraceID it started giving unequal address error as displayed in above comment. This is because (according to my analysis) we are passing model.TraceID in multiread but the query builder is expecting parameters in dbmodel.TraceID causing change in the address. Therefore I made the mock independent of these addresses!
|
@yurishkuro Have given a try to refactor the |
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
internal/storage/v1/elasticsearch/spanstore/internal/dbmodel/model.go
Outdated
Show resolved
Hide resolved
internal/storage/v1/elasticsearch/spanstore/internal/dbmodel/to_domain.go
Outdated
Show resolved
Hide resolved
|
Going to fix the tests! |
Signed-off-by: Manik2708 <[email protected]>
internal/storage/v1/elasticsearch/spanstore/internal/dbmodel/to_domain.go
Show resolved
Hide resolved
internal/storage/v1/elasticsearch/spanstore/internal/dbmodel/to_domain.go
Show resolved
Hide resolved
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
| @@ -222,35 +220,14 @@ func timeRangeIndices(indexName, indexDateLayout string, startTime time.Time, en | |||
| return indices | |||
| } | |||
|
|
|||
| // GetTrace takes a traceID and returns a Trace associated with that traceID | |||
There was a problem hiding this comment.
I don't understand any of these changes anymore. We said multiple times - keep the diffs to a minimum. This file is 75% rewritten, where all we needed is to slightly change the entry point signatures.
There was a problem hiding this comment.
But it was asked in the comment #6824 (comment) to move SpanReaderV1 in new file. That's what I was aiming for!
There was a problem hiding this comment.
Yes because reader v1 is a net new code which is supposed to be a very thin shim over the main reader. But you are introducing massive changes for the main reader, which should only have cosmetic changes to the signatures of the entry points.
There was a problem hiding this comment.
Suggestion - start with get services / operations only to get the refactoring in place, and for other functions keep the signatures the same between v1 and main writer, so just a straight pass through.
There was a problem hiding this comment.
this is what I am expecting to see:
- new readerv1 struct that has the same public methods as
readerand implements the official storage API. - each method performs a translation of the arguments and delegates to the
reader. Also translates the results back intomodel/* - the existing
readerhas almost no changes except all its public methods are modified to accept/returndbmodeltypes. NO OTHER CHANGES are needed to the existing reader.
This reverts commit d1155d2. Signed-off-by: Manik2708 <[email protected]>
This reverts commit db96536. Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
@yurishkuro Thanks for the explanation! I have given a try, and now all public methods of |
Signed-off-by: Manik2708 <[email protected]>
| @@ -223,34 +222,29 @@ func timeRangeIndices(indexName, indexDateLayout string, startTime time.Time, en | |||
| } | |||
|
|
|||
| // GetTrace takes a traceID and returns a Trace associated with that traceID | |||
| func (s *SpanReader) GetTrace(ctx context.Context, query spanstore.GetTraceParameters) (*model.Trace, error) { | |||
| func (s *SpanReader) GetTrace(ctx context.Context, traceIDs map[dbmodel.TraceID]string) (map[dbmodel.TraceID][]*dbmodel.Span, error) { | |||
There was a problem hiding this comment.
traceIDs map[dbmodel.TraceID]string
what is this a map into? What are the string values?
There was a problem hiding this comment.
String values are the legacy trace ids. Please follow this implementation of legacy trace id:
func getLegacyTraceId(traceID model.TraceID) string {
// https://github.com/jaegertracing/jaeger/pull/1956 added leading zeros to IDs
// So we need to also read IDs without leading zeros for compatibility with previously saved data.
// TODO remove in newer versions, added in Jaeger 1.16
if traceID.High == 0 {
return strconv.FormatUint(traceID.Low, 16)
}
return fmt.Sprintf("%x%016x", traceID.High, traceID.Low)
}
The logic is not changed, it is directly taken from here
jaeger/internal/storage/v1/elasticsearch/spanstore/reader.go
Lines 467 to 475 in 6037db2
| func (s *SpanReader) collectSpans(esSpansRaw []*elastic.SearchHit) ([]*model.Span, error) { | ||
| spans := make([]*model.Span, len(esSpansRaw)) | ||
|
|
||
| func (s *SpanReader) collectJsonSpans(esSpansRaw []*elastic.SearchHit) ([]*dbmodel.Span, error) { |
There was a problem hiding this comment.
| func (s *SpanReader) collectJsonSpans(esSpansRaw []*elastic.SearchHit) ([]*dbmodel.Span, error) { | |
| func (s *SpanReader) collectSpans(esSpansRaw []*elastic.SearchHit) ([]*dbmodel.Span, error) { |
unnecessary renaming
Which problem is this PR solving?
Fixes a part of: #6458
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test