Use elasticsearch bulk API #656

pavolloffay · 2018-01-18T14:40:24Z

Fixes #439

Add BulkService API for storing documents. It caches requests until a threshold is reached - size, time, number of operations. Then it reports bulk to ES.

Performance: store 300k spans
Without bulk API ~15min
WIth bulk API: ~40s

This currently removes metrics, once this is merged I will do PR to include stats https://github.com/olivere/elastic/wiki/BulkProcessor#stats

Signed-off-by: Pavol Loffay ploffay@redhat.com

coveralls · 2018-01-18T14:50:41Z

Coverage decreased (-0.1%) to 99.9% when pulling 3c9cdb5 on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

coveralls · 2018-01-18T15:20:15Z

Coverage decreased (-0.1%) to 99.9% when pulling 5d979eb on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

coveralls · 2018-01-18T15:36:05Z

Coverage decreased (-0.07%) to 99.929% when pulling 9077a2f on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

coveralls · 2018-01-18T15:45:59Z

Coverage decreased (-0.07%) to 99.929% when pulling 7b758db on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

coveralls · 2018-01-18T15:49:08Z

Coverage decreased (-0.07%) to 99.929% when pulling 7b758db on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

yurishkuro · 2018-01-18T15:58:51Z

could you elaborate in the description what exactly this change is doing? The SpanWriter API is still accepting one span at a time, so what do we mean by bulk? Does the client now batch the write requests? If so, how does this affect the success/failure metrics emitted?

pavolloffay · 2018-01-18T16:10:33Z

API is collecting spans until a threshold is reached - size, number of operations, time. Then it reports what it has in the buffer.

I have removed metrics for now e.g. span write duration as there is no way how to find out the number. There is a way how to get stats from bulk service, but that is not per index name (e.g. spans, dependencies). I would prefer to add it in a separate PR https://github.com/olivere/elastic/wiki/BulkProcessor#stats

coveralls · 2018-01-18T16:26:33Z

Coverage remained the same at 100.0% when pulling ba28480 on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

yurishkuro

Is the bulk functionality always going to be on, or configurable?

yurishkuro · 2018-01-18T15:48:14Z

Makefile

+.PHONY: install-mockery
+install-mockery:
+	go get github.com/vektra/mockery
+	go build github.com/vektra/mockery


go get should install it already

yurishkuro · 2018-01-18T16:27:33Z

plugin/storage/es/spanstore/writer_test.go

@@ -235,75 +197,6 @@ func TestSpanIndexName(t *testing.T) {
 	assert.Equal(t, "jaeger-service-1995-04-21", serviceIndexName)
 }

-func TestCheckAndCreateIndex(t *testing.T) {


what's the argument for deleting this test?

It does not affect test coverage, I think some of them were repeating stuff

yurishkuro · 2018-01-18T16:29:24Z

awesome perf bump

coveralls · 2018-01-18T16:36:20Z

Coverage remained the same at 100.0% when pulling ba28480 on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

pavolloffay · 2018-01-18T16:49:35Z

Bulk API is always enabled. There are flags to configure it e.g. flush interval, size or flush period.

pavolloffay · 2018-01-18T16:56:17Z

pkg/es/client.go

@@ -27,6 +27,7 @@ type Client interface {
 	Index() IndexService
 	Search(indices ...string) SearchService
 	MultiSearch() MultiSearchService
+	Close() error


What about embedding io.Closer here?

I would like to close this in in main case of sigterm, however there is present only spanwriter. Do we want to add close to spanwriter too?

Lately I prefer not adding Close to interfaces. The main often creates services with explicit type, i.e. *SomeStruct, in which case it already has access to Close(). In cases when we use an abstract factory pattern and get an interface back, we can still call close via conditional cast:

if closer, ok := someComponent.(io.Closer); ok { closer.Close() }

I had to add close here to be able close the bulk service it in es span writer.

coveralls · 2018-01-18T17:03:24Z

Coverage remained the same at 100.0% when pulling b9ce4a2 on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

mabn · 2018-01-18T17:21:22Z

plugin/storage/es/options.go

+				BulkSize:          5 * 1000 * 1000,
+				BulkWorkers:       1,
+				BulkActions:       1000,
+				BulkFlushInterval: 0,


Maybe set the default to 100ms?
I think that with 0 spans might not be flushed if someone is just testing and sends less than 1000 spans. Or the added delay will be high at low throughputs.

100ms seems quite low to leverage bulk API. If somebody is testing even 1s could be fine, until you open a browser and refresh spans will be there.

Here are some tests, flush interval: test duration
100ms: 100s
200ms: 80s
300ms: 77s
500ms: ~76s
1s: ~75s

I would go with at least 200ms

coveralls · 2018-01-19T09:12:07Z

Coverage decreased (-0.1%) to 99.9% when pulling 020e8fa on pavolloffay:es-bulk-api into 8c55478 on jaegertracing:master.

coveralls · 2018-01-19T13:43:03Z

Coverage remained the same at 100.0% when pulling d03fea6 on pavolloffay:es-bulk-api into 5d13dcd on jaegertracing:master.

pavolloffay · 2018-01-19T14:48:17Z

It should be ready for review

yurishkuro · 2018-01-19T21:30:34Z

pkg/es/config/config.go

+	BulkSize          int
+	BulkWorkers       int
+	BulkActions       int
+	BulkFlushInterval time.Duration


do we want to add yaml tags for these? Because they are multi-word names, they will look differently otherwise

where are these tags used?

yurishkuro · 2018-01-19T21:33:09Z

do we cover the insert behavior in the integration tests with real ES?

pavolloffay · 2018-01-22T16:42:38Z

do you mean store and query data? Then yes https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/integration/es_integration_test.go#L100

black-adder · 2018-01-23T04:30:03Z

pkg/es/wrapper.go

 }

 // Id calls this function to internal service.
 func (i ESIndexService) Id(id string) IndexService {
-	return WrapESIndexService(i.indexService.Id(id))
+	a := WrapESIndexService(i.bulkIndexReq.Id(id), i.bulkService)


nit: inline

black-adder · 2018-01-23T04:30:49Z

pkg/es/wrapper.go

-// Do calls this function to internal service.
-func (i ESIndexService) Do(ctx context.Context) (*elastic.IndexResponse, error) {
-	return i.indexService.Do(ctx)
+// Add add the request to bulk service


s/Add add/Add adds

yurishkuro · 2018-01-23T04:56:44Z

I suggest rebasing from master, there is a fix for flaky travis install

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2018-01-23T09:05:10Z

I will merge, if there are more comments I will apply them in a separate PR

pavolloffay requested review from black-adder, vprithvi and yurishkuro as code owners January 18, 2018 14:40

ghost assigned pavolloffay Jan 18, 2018

ghost added the review label Jan 18, 2018

yurishkuro reviewed Jan 18, 2018

View reviewed changes

pavolloffay commented Jan 18, 2018

View reviewed changes

mabn reviewed Jan 18, 2018

View reviewed changes

yurishkuro reviewed Jan 19, 2018

View reviewed changes

pavolloffay force-pushed the es-bulk-api branch from b59f73b to 47c4587 Compare January 22, 2018 16:43

black-adder approved these changes Jan 23, 2018

View reviewed changes

Use elasticsearch bulk API

a5fca44

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the es-bulk-api branch from 47c4587 to a5fca44 Compare January 23, 2018 08:35

Fix review comments

d03fea6

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay mentioned this pull request Jan 23, 2018

Consume stats from ES bulk service #662

Open

pavolloffay merged commit e52ecff into jaegertracing:master Jan 23, 2018

ghost removed the review label Jan 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use elasticsearch bulk API #656

Use elasticsearch bulk API #656

pavolloffay commented Jan 18, 2018 •

edited

Loading

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018

yurishkuro commented Jan 18, 2018

pavolloffay commented Jan 18, 2018

coveralls commented Jan 18, 2018

yurishkuro left a comment

yurishkuro Jan 18, 2018

yurishkuro Jan 18, 2018

pavolloffay Jan 18, 2018

yurishkuro commented Jan 18, 2018

coveralls commented Jan 18, 2018

pavolloffay commented Jan 18, 2018

pavolloffay Jan 18, 2018

yurishkuro Jan 18, 2018

pavolloffay Jan 19, 2018

coveralls commented Jan 18, 2018

mabn Jan 18, 2018

pavolloffay Jan 19, 2018

coveralls commented Jan 19, 2018

coveralls commented Jan 19, 2018 •

edited

Loading

pavolloffay commented Jan 19, 2018

yurishkuro Jan 19, 2018

pavolloffay Jan 22, 2018

yurishkuro commented Jan 19, 2018

pavolloffay commented Jan 22, 2018

black-adder Jan 23, 2018

black-adder Jan 23, 2018

yurishkuro commented Jan 23, 2018

pavolloffay commented Jan 23, 2018

Use elasticsearch bulk API #656

Use elasticsearch bulk API #656

Conversation

pavolloffay commented Jan 18, 2018 • edited Loading

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018

yurishkuro commented Jan 18, 2018

pavolloffay commented Jan 18, 2018

coveralls commented Jan 18, 2018

yurishkuro 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

yurishkuro commented Jan 18, 2018

coveralls commented Jan 18, 2018

pavolloffay commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 19, 2018

coveralls commented Jan 19, 2018 • edited Loading

pavolloffay commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jan 19, 2018

pavolloffay commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jan 23, 2018

pavolloffay commented Jan 23, 2018

pavolloffay commented Jan 18, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading