The implementation of FindTraceIDs function for ElasticSearch reader. #1280

vlamug · 2019-01-11T14:36:39Z

Related to #1241

Which problem is this PR solving?

It allows to use FindTraceIDs function for ElasticSearch storage.

Short description of the changes

The implementations of FindTraceIDs function for ElasticSearch was added.

codecov · 2019-01-11T14:46:31Z

Codecov Report

Merging #1280 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1280   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         162     162           
  Lines        7265    7277   +12     
======================================
+ Hits         7265    7277   +12

Impacted Files	Coverage Δ
plugin/storage/es/spanstore/reader.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6115665...49a132a. Read the comment docs.

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

vprithvi

Thanks for the PR! I'm curious about how we can reuse existing code around findTraceIDs

cc @pavolloffay

vprithvi · 2019-01-11T18:29:44Z

plugin/storage/es/spanstore/reader.go

 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 {


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

Ok, thanks.

vprithvi · 2019-01-11T18:34:58Z

plugin/storage/es/spanstore/reader.go

+
+	traceIDs := []model.TraceID{}
+	for _, ID := range esTraceIDs {
+		if len(traceIDs) >= traceQuery.NumTraces {


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

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.

vprithvi · 2019-01-11T18:35:51Z

plugin/storage/es/spanstore/reader.go

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


nit: Could you log the traceID string?

Good suggestion. Thanks.

mkabischev · 2019-01-14T09:39:03Z

plugin/storage/es/spanstore/reader.go

+		return nil, err
+	}
+
+	traceIDs := []model.TraceID{}


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

I agree. Thanks.

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

vlamug · 2019-01-15T12:52:23Z

@vprithvi , @mkabischev , guys, the changes were added. Please, check it. Thanks.

vprithvi · 2019-01-17T23:05:27Z

plugin/storage/es/spanstore/reader.go

@@ -317,6 +329,20 @@ func (s *SpanReader) multiRead(ctx context.Context, traceIDs []string, startTime
 	return traces, nil
 }

+func (s *SpanReader) validateQueryAndFindTraceIDs(ctx context.Context, traceQuery *spanstore.TraceQueryParameters) ([]string, error) {


Looks good!

vprithvi · 2019-01-17T23:06:20Z

plugin/storage/es/spanstore/reader_test.go

@@ -668,11 +668,142 @@ func TestFindTraceIDs(t *testing.T) {
 	testGet(traceIDAggregation, t)
 }

-func TestFindTraceIDNotImplemented(t *testing.T) {
+func TestSpanReader_TestFindTraceIDs(t *testing.T) {


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

Yes, of course, I will make change soon.

pavolloffay

We should add some integration tests to test this.

pavolloffay · 2019-01-18T13:32:23Z

plugin/storage/es/spanstore/reader.go

@@ -317,6 +329,20 @@ func (s *SpanReader) multiRead(ctx context.Context, traceIDs []string, startTime
 	return traces, nil
 }

+func (s *SpanReader) validateQueryAndFindTraceIDs(ctx context.Context, traceQuery *spanstore.TraceQueryParameters) ([]string, error) {
+	childSpan, _ := opentracing.StartSpanFromContext(ctx, "validateQueryAndFindTraceIDs")


I don't think we need a span for this operation.

Ok, thanks, I will delete it.

please delete it

pavolloffay · 2019-01-18T13:34:55Z

The integration tests can be done in a separate PR. This PR LGTM. It would be nice to reuse tests if possbile though as @vprithvi mentioned

vlamug · 2019-01-23T07:24:46Z

Hello @vprithvi. Sorry for the late answer. I researched the possibility to refactor tests, but I do not have any idea how to do it right in such case. For example the method TestSpanReader_FindTraces has hits, but the method TestSpanReader_TestFindTraceIDs doest not. Then the method TestSpanReader_FindTraces makes call to mockMultiSearchService, but method TestSpanReader_TestFindTraceIDs does not. So, If I extract the similar functionality into separate function, it function will have many conditions: some for find traces methods, some for find trace ids methods.
If you have any suggestion, please let me know and I will make changes.
Thanks.

vlamug · 2019-01-28T08:33:21Z

@vprithvi , hi) Could you please check the last update?)
We are waiting for the changes in this pull request in order to use FindTraceIDs function.

A lot of thanks.

vprithvi

@vlamug While going through the code and tests, it seems that there might be a simpler way of implementing.

Could we have validateQueryAndFindTraceIDs return []model.TraceID, error instead of []string, error?

If we do this, we'll need to update multiRead to accept []model.TraceID instead of string, and update some maps inside multiRead to be indexed by model.TraceID instead of string.

On the plus side, we simplify tests because FindTraces will be using the same code paths.
FindTraceIDs could simply do:

return s.validateQueryAndFindTraceIDs(ctx, traceQuery)

and even better, we can rename validateQueryAndFindTraceIDs to FindTraceIDs.

Thanks for your contribution and the ping!

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

… function. Signed-off-by: vlamug <vmugultyanov@ozon.ru>

…to find_trace_ids_for_es_reader

vprithvi

Looking good!

Because FindTraceIDs is used by FindTraces, the old tests cover it's functionality well and thus we can remove nearly all new tests.
The new functionality that FindTraceIDs provides is converting []string to []model.TraceIDs, which we can test more easily by refactoring into something like func convertTraceStringsToTraceID([]string) ([]model.TraceID, error) and just testing that!

vprithvi · 2019-01-29T16:28:14Z

plugin/storage/es/spanstore/reader.go

@@ -377,6 +395,7 @@ func validateQuery(p *spanstore.TraceQueryParameters) error {
 func (s *SpanReader) findTraceIDs(ctx context.Context, traceQuery *spanstore.TraceQueryParameters) ([]string, error) {
 	childSpan, _ := opentracing.StartSpanFromContext(ctx, "findTraceIDs")
 	defer childSpan.Finish()
+


nit: extraneous change

Ok, thanks!

vprithvi · 2019-01-29T17:58:35Z

plugin/storage/es/spanstore/reader.go

@@ -333,18 +352,17 @@ func (s *SpanReader) multiRead(ctx context.Context, traceIDs []string, startTime
 				return nil, err


I noticed that the integration tests failed. Perhaps L298 requires a traceID.String()?

Fixed. Thanks 👍

… to "convertTraceStringsToTraceID" function. Signed-off-by: vlamug <vmugultyanov@ozon.ru>

…e of traceID models to slice of strings. Signed-off-by: vlamug <vmugultyanov@ozon.ru>

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

vlamug · 2019-01-30T10:28:15Z

@vprithvi hello, thanks for your check and suggestion according refactoring. I fixed the integrations tests, but the ci tests were failed(https://travis-ci.org/jaegertracing/jaeger/jobs/486321151). I just run make cover command locally and all tests were finished with success result. Could you help me to understand what the problem is in these tests?
Many thanks!

vprithvi · 2019-01-30T15:38:35Z

plugin/storage/es/spanstore/reader_test.go

-		assert.Nil(t, traceIDs)
-		assert.EqualError(t, err, "not implemented")
-	})
+func TestTraceIDsStringsToModelsConvertation(t *testing.T) {


nit: typo: s/convertation/conversion

vprithvi · 2019-01-30T15:38:41Z

plugin/storage/es/spanstore/reader_test.go

+	assert.Equal(t, 0, len(traceIDs))
+}
+
+func TestTraceIDsModelsToStringsConvertation(t *testing.T) {


nit: typo: s/convertation/conversion

vprithvi · 2019-01-30T15:42:52Z

plugin/storage/es/spanstore/reader.go


 	childSpan, _ := opentracing.StartSpanFromContext(ctx, "multiRead")
-	childSpan.LogFields(otlog.Object("trace_ids", traceIDs))
+	childSpan.LogFields(otlog.Object("trace_ids", convertTraceIDsModelsToStrings(traceIDs)))


Is this conversion necessary? The logging code is implemented as such:

func (ml fieldsAsMap) EmitObject(key string, value interface{}) { ml[key] = fmt.Sprintf("%+v", value) }

which should print out traceIDs properly without doing additional work.

Oh, I see. I just checked it and it really prints trace id is before. Ok, I will remove it. Thank you.

vprithvi · 2019-01-30T15:44:43Z

@vlamug It looks like the failure wasn't related to your change! Once the question about the string conversion is answered, we can merge!

…edundant. Signed-off-by: vlamug <vmugultyanov@ozon.ru>

…to find_trace_ids_for_es_reader

vlamug · 2019-01-31T08:21:35Z

@vprithvi , thank you for review. The all needed changes were made.

vlamug requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners January 11, 2019 14:36

vlamug changed the title ~~The implementation of Find trace ids for es reader~~ The implementation of FindTraceIDs function for ElasticSearch reader. Jan 11, 2019

vlamug added 3 commits January 11, 2019 18:54

The implementations of "FindTraceIDs" function for ElasticSearch reader.

23bbaf5

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

The using validation of query and added default numTraces value.

c55fde1

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

The tests for FindTraceIDs were added.

4350e3a

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

vlamug force-pushed the find_trace_ids_for_es_reader branch from 470f16f to 4350e3a Compare January 11, 2019 14:54

The test for checking incorrect traceID.

0ace6df

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

vlamug force-pushed the find_trace_ids_for_es_reader branch from 15b1e4d to 0ace6df Compare January 11, 2019 15:20

The tests are fixed.

d4a2644

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

vprithvi suggested changes Jan 11, 2019

View reviewed changes

mkabischev reviewed Jan 14, 2019

View reviewed changes

vlamug added 2 commits January 15, 2019 16:39

The improvements by code-review.

e90848d

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

The new function validateQueryAndFindTraceIDs.

ae75936

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

Merge branch 'master' into find_trace_ids_for_es_reader

8a1ec2e

pavolloffay added the storage/elasticsearch label Jan 16, 2019

vprithvi reviewed Jan 17, 2019

View reviewed changes

pavolloffay reviewed Jan 18, 2019

View reviewed changes

Merge branch 'master' into find_trace_ids_for_es_reader

0c399da

ghost assigned vprithvi Jan 28, 2019

ghost added the review label Jan 28, 2019

vprithvi approved these changes Jan 28, 2019

View reviewed changes

vprithvi suggested changes Jan 28, 2019

View reviewed changes

vlamug added 3 commits January 29, 2019 18:57

The span validateQueryAndFindTraceIDs was removed.

424bc31

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

The using slice of model.TraceID instead slice of string in multiRead…

5aae75f

… function. Signed-off-by: vlamug <vmugultyanov@ozon.ru>

Merge remote-tracking branch 'origin/find_trace_ids_for_es_reader' in…

7a70584

…to find_trace_ids_for_es_reader

vprithvi reviewed Jan 29, 2019

View reviewed changes

vlamug added 3 commits January 30, 2019 11:38

The converting slice of string to slice of mode.TraceID was extracted…

2600634

… to "convertTraceStringsToTraceID" function. Signed-off-by: vlamug <vmugultyanov@ozon.ru>

The method "convertTraceIDsModelsToStrings" was added to convert slic…

b108809

…e of traceID models to slice of strings. Signed-off-by: vlamug <vmugultyanov@ozon.ru>

The fix of passing traceID into NewTermQuery function.

debd4d6

Signed-off-by: vlamug <vmugultyanov@ozon.ru>

Merge branch 'master' into find_trace_ids_for_es_reader

66f1851

vprithvi reviewed Jan 30, 2019

View reviewed changes

vlamug added 2 commits January 31, 2019 11:25

The convertTraceIDsModelsToStrings method was remove, because it is r…

75c30d3

…edundant. Signed-off-by: vlamug <vmugultyanov@ozon.ru>

Merge remote-tracking branch 'origin/find_trace_ids_for_es_reader' in…

0e7139a

…to find_trace_ids_for_es_reader

vprithvi approved these changes Jan 31, 2019

View reviewed changes

Merge branch 'master' into find_trace_ids_for_es_reader

49a132a

vprithvi merged commit d40ea34 into jaegertracing:master Jan 31, 2019

ghost removed the review label Jan 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The implementation of FindTraceIDs function for ElasticSearch reader. #1280

The implementation of FindTraceIDs function for ElasticSearch reader. #1280

vlamug commented Jan 11, 2019 •

edited by pavolloffay

Loading

codecov bot commented Jan 11, 2019 •

edited

Loading

vprithvi left a comment

vprithvi Jan 11, 2019

vlamug Jan 14, 2019

vprithvi Jan 11, 2019

vlamug Jan 14, 2019

vprithvi Jan 11, 2019

vlamug Jan 14, 2019

mkabischev Jan 14, 2019

vlamug Jan 14, 2019

vlamug commented Jan 15, 2019

vprithvi Jan 17, 2019

vprithvi Jan 17, 2019

vlamug Jan 21, 2019

pavolloffay left a comment

pavolloffay Jan 18, 2019

vlamug Jan 21, 2019

yurishkuro Jan 28, 2019

pavolloffay commented Jan 18, 2019

vlamug commented Jan 23, 2019

vlamug commented Jan 28, 2019

vprithvi left a comment •

edited

Loading

vprithvi left a comment

vprithvi Jan 29, 2019

vlamug Jan 30, 2019

vprithvi Jan 29, 2019

vlamug Jan 30, 2019

vlamug commented Jan 30, 2019

vprithvi Jan 30, 2019

vprithvi Jan 30, 2019

vprithvi Jan 30, 2019

vlamug Jan 31, 2019

vprithvi commented Jan 30, 2019

vlamug commented Jan 31, 2019

		@@ -333,18 +352,17 @@ func (s *SpanReader) multiRead(ctx context.Context, traceIDs []string, startTime
		return nil, err

The implementation of FindTraceIDs function for ElasticSearch reader. #1280

The implementation of FindTraceIDs function for ElasticSearch reader. #1280

Conversation

vlamug commented Jan 11, 2019 • edited by pavolloffay Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jan 11, 2019 • edited Loading

Codecov Report

vprithvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlamug commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Jan 18, 2019

vlamug commented Jan 23, 2019

vlamug commented Jan 28, 2019

vprithvi left a comment • edited Loading

Choose a reason for hiding this comment

vprithvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlamug commented Jan 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vprithvi commented Jan 30, 2019

vlamug commented Jan 31, 2019

vlamug commented Jan 11, 2019 •

edited by pavolloffay

Loading

codecov bot commented Jan 11, 2019 •

edited

Loading

vprithvi left a comment •

edited

Loading