Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The implementation of FindTraceIDs function for ElasticSearch reader. #1280

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
23bbaf5
The implementations of "FindTraceIDs" function for ElasticSearch reader.
Jan 11, 2019
c55fde1
The using validation of query and added default numTraces value.
Jan 11, 2019
4350e3a
The tests for FindTraceIDs were added.
Jan 11, 2019
0ace6df
The test for checking incorrect traceID.
Jan 11, 2019
d4a2644
The tests are fixed.
Jan 11, 2019
e90848d
The improvements by code-review.
Jan 15, 2019
ae75936
The new function validateQueryAndFindTraceIDs.
Jan 15, 2019
8a1ec2e
Merge branch 'master' into find_trace_ids_for_es_reader
vlamug Jan 16, 2019
0c399da
Merge branch 'master' into find_trace_ids_for_es_reader
vprithvi Jan 28, 2019
424bc31
The span validateQueryAndFindTraceIDs was removed.
Jan 29, 2019
5aae75f
The using slice of model.TraceID instead slice of string in multiRead…
Jan 29, 2019
7a70584
Merge remote-tracking branch 'origin/find_trace_ids_for_es_reader' in…
Jan 29, 2019
2600634
The converting slice of string to slice of mode.TraceID was extracted…
Jan 30, 2019
b108809
The method "convertTraceIDsModelsToStrings" was added to convert slic…
Jan 30, 2019
debd4d6
The fix of passing traceID into NewTermQuery function.
Jan 30, 2019
66f1851
Merge branch 'master' into find_trace_ids_for_es_reader
vprithvi Jan 30, 2019
75c30d3
The convertTraceIDsModelsToStrings method was remove, because it is r…
Jan 31, 2019
0e7139a
Merge remote-tracking branch 'origin/find_trace_ids_for_es_reader' in…
Jan 31, 2019
49a132a
Merge branch 'master' into find_trace_ids_for_es_reader
vprithvi Jan 31, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions plugin/storage/es/spanstore/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,35 @@ func (s *SpanReader) FindTraces(ctx context.Context, traceQuery *spanstore.Trace
return s.multiRead(ctx, uniqueTraceIDs, traceQuery.StartTimeMin, traceQuery.StartTimeMax)
}

// FindTraceIDs is not implemented.
// FindTraceIDs retrieves traces IDs that match the traceQuery
func (s *SpanReader) FindTraceIDs(ctx context.Context, traceQuery *spanstore.TraceQueryParameters) ([]model.TraceID, error) {
return nil, errors.New("not implemented") // TODO: Implement
if err := validateQuery(traceQuery); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a lot of repeated code between FindTraces and FindTraceIDs, are we able to refactor such that the initial validation and retrieval are shared?

Also, could you create a new span here - similar to L221

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks.

return nil, err
}
if traceQuery.NumTraces == 0 {
traceQuery.NumTraces = defaultNumTraces
}

esTraceIDs, err := s.findTraceIDs(ctx, traceQuery)
if err != nil {
return nil, err
}

traceIDs := []model.TraceID{}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know the length of result slice, so you can preallocate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Thanks.

for _, ID := range esTraceIDs {
if len(traceIDs) >= traceQuery.NumTraces {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this required? I see that findTraceIDs already takes in traceQuery.NumTraces. Does it return a larger number of traces than specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I studied the implementation of such function for cassandra and there are some troubles with it, therefore the implementation for cassandra uses this condition. It seems that here it is ok. I will remove it.

break
}

traceID, err := model.TraceIDFromString(ID)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could you log the traceID string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Thanks.

}

traceIDs = append(traceIDs, traceID)
}

return traceIDs, nil
}

func (s *SpanReader) multiRead(ctx context.Context, traceIDs []string, startTime, endTime time.Time) ([]*model.Trace, error) {
Expand Down
137 changes: 134 additions & 3 deletions plugin/storage/es/spanstore/reader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -668,11 +668,142 @@ func TestFindTraceIDs(t *testing.T) {
testGet(traceIDAggregation, t)
}

func TestFindTraceIDNotImplemented(t *testing.T) {
func TestSpanReader_TestFindTraceIDs(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests look very similar to the TestSpanReader_TestFindTraces, ... tests. Is there a way to DRY them up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course, I will make change soon.

goodAggregations := make(map[string]*json.RawMessage)
rawMessage := []byte(`{"buckets": [{"key": "1","doc_count": 16},{"key": "2","doc_count": 16},{"key": "3","doc_count": 16}]}`)
goodAggregations[traceIDAggregation] = (*json.RawMessage)(&rawMessage)

withSpanReader(func(r *spanReaderTest) {
// find trace IDs
mockSearchService(r).Return(&elastic.SearchResult{Aggregations: elastic.Aggregations(goodAggregations)}, nil)

traceIDsQuery := &spanstore.TraceQueryParameters{
ServiceName: serviceName,
Tags: map[string]string{
"hello": "world",
},
StartTimeMin: time.Now().Add(-1 * time.Hour),
StartTimeMax: time.Now(),
NumTraces: 1,
}

traceIDs, err := r.reader.FindTraceIDs(context.Background(), traceIDsQuery)
require.NoError(t, err)
assert.Len(t, traceIDs, 1)
assert.EqualValues(t, 1, traceIDs[0].Low)
})
}

func TestSpanReader_FindTraceIDsInvalidQuery(t *testing.T) {
goodAggregations := make(map[string]*json.RawMessage)
rawMessage := []byte(`{"buckets": [{"key": "1","doc_count": 16},{"key": "2","doc_count": 16},{"key": "3","doc_count": 16}]}`)
goodAggregations[traceIDAggregation] = (*json.RawMessage)(&rawMessage)

withSpanReader(func(r *spanReaderTest) {
traceIDs, err := r.reader.FindTraceIDs(context.Background(), nil)
mockSearchService(r).Return(&elastic.SearchResult{Aggregations: elastic.Aggregations(goodAggregations)}, nil)

traceIDsQuery := &spanstore.TraceQueryParameters{
ServiceName: "",
Tags: map[string]string{
"hello": "world",
},
StartTimeMin: time.Now().Add(-1 * time.Hour),
StartTimeMax: time.Now(),
}

traceIDs, err := r.reader.FindTraceIDs(context.Background(), traceIDsQuery)
require.Error(t, err)
assert.Nil(t, traceIDs)
})
}

func TestSpanReader_FindTraceIDsAggregationFailure(t *testing.T) {
goodAggregations := make(map[string]*json.RawMessage)

withSpanReader(func(r *spanReaderTest) {
mockSearchService(r).Return(&elastic.SearchResult{Aggregations: elastic.Aggregations(goodAggregations)}, nil)

traceIDsQuery := &spanstore.TraceQueryParameters{
ServiceName: serviceName,
Tags: map[string]string{
"hello": "world",
},
StartTimeMin: time.Now().Add(-1 * time.Hour),
StartTimeMax: time.Now(),
}

traceIDs, err := r.reader.FindTraceIDs(context.Background(), traceIDsQuery)
require.Error(t, err)
assert.Nil(t, traceIDs)
assert.EqualError(t, err, "not implemented")
})
}

func TestSpanReader_FindTraceIDsNoTraceIDs(t *testing.T) {
goodAggregations := make(map[string]*json.RawMessage)
rawMessage := []byte(`{"buckets": []}`)
goodAggregations[traceIDAggregation] = (*json.RawMessage)(&rawMessage)

withSpanReader(func(r *spanReaderTest) {
mockSearchService(r).Return(&elastic.SearchResult{Aggregations: elastic.Aggregations(goodAggregations)}, nil)

traceIDsQuery := &spanstore.TraceQueryParameters{
ServiceName: serviceName,
Tags: map[string]string{
"hello": "world",
},
StartTimeMin: time.Now().Add(-1 * time.Hour),
StartTimeMax: time.Now(),
}

traceIDs, err := r.reader.FindTraceIDs(context.Background(), traceIDsQuery)
require.NoError(t, err)
assert.Len(t, traceIDs, 0)
})
}

func TestSpanReader_FindTraceIDsReadTraceIDsFailure(t *testing.T) {
goodAggregations := make(map[string]*json.RawMessage)
rawMessage := []byte(`{"buckets": [{"key": "1","doc_count": 16},{"key": "2","doc_count": 16}]}`)
goodAggregations[traceIDAggregation] = (*json.RawMessage)(&rawMessage)

withSpanReader(func(r *spanReaderTest) {
mockSearchService(r).Return(nil, errors.New("read error"))

traceIDsQuery := &spanstore.TraceQueryParameters{
ServiceName: serviceName,
Tags: map[string]string{
"hello": "world",
},
StartTimeMin: time.Now().Add(-1 * time.Hour),
StartTimeMax: time.Now(),
}

traceIDs, err := r.reader.FindTraceIDs(context.Background(), traceIDsQuery)
require.EqualError(t, err, "Search service failed: read error")
assert.Len(t, traceIDs, 0)
})
}

func TestSpanReader_FindTraceIDsIncorrectTraceIDFailure(t *testing.T) {
goodAggregations := make(map[string]*json.RawMessage)
rawMessage := []byte(`{"buckets": [{"key": "sdfsdfdssd234nsdvsdfldjsf","doc_count": 16},{"key": "2","doc_count": 16}]}`)
goodAggregations[traceIDAggregation] = (*json.RawMessage)(&rawMessage)

withSpanReader(func(r *spanReaderTest) {
mockSearchService(r).Return(&elastic.SearchResult{Aggregations: elastic.Aggregations(goodAggregations)}, nil)

traceIDsQuery := &spanstore.TraceQueryParameters{
ServiceName: serviceName,
Tags: map[string]string{
"hello": "world",
},
StartTimeMin: time.Now().Add(-1 * time.Hour),
StartTimeMax: time.Now(),
}

traceIDs, err := r.reader.FindTraceIDs(context.Background(), traceIDsQuery)
require.EqualError(t, err, `strconv.ParseUint: parsing "sdfsdfdss": invalid syntax`)
assert.Len(t, traceIDs, 0)
})
}

Expand Down