Add native OTEL ES exporter #2295

pavolloffay · 2020-06-18T15:10:51Z

This PR reimplements OpenTelemetry Elasticsarch exporter. Only span exporter/writer is implemented the query still uses the current ES storage implementation.

Notable changes:

new span writer implementation versioned in exporter/jaegerelasticsearch
used ES client is https://github.com/elastic/go-elasticsearch
OTEL data model is directly transformed to Jaeger ES model
writer uses ES Bulk API
batching and retries capabilities will be used as OTEL processors (batch, queued_retry)
ES client is abstracted and specific implementation is created based on the supplied ES version or derived from ES ping API at startup. This allows us to easily add support for new ES versions
added integration tests

Other changes to the codebase:

storage integration test is more generic - it can be executed from any package

TODOs:

ES clients tests
enable integration storage test on travis
ES exporter should probably accept its own params struct and not config.Configuration

Considerations:

OTEL batch processor can overflow configured max batch size (units) Batch processor returns a bigger batch than configured open-telemetry/opentelemetry-collector#1140
OTEL batch processor does not have configurable batch size in bytes https://github.com/open-telemetry/opentelemetry-collector/issues/1139
OTEL queued retry processor cannot retry a single index unit - only a part of ES bulk API can fail. Allow partial retries of trace data in queued retry processor open-telemetry/opentelemetry-collector#990
when OTEL queued retry is used the retried spans will be serialized multiple times.

Performance

From the perf tests I have done the new writer performs well (with batch processor). It seems to be slightly faster than the current all-in-one with similar settings.

docker run -it --rm -e "ES_JAVA_OPTS=-Xms2g -Xmx2g" -p 9200:9200 -p 9300:9300 -e "http.host=0.0.0.0" -e "discovery.type=single-node" --name=elasticsearch docker.elastic.co/elasticsearch/elasticsearch-oss:6.8.4

Jaeger OTEL all-in-one with ES exporter and batch processor

processors:
  batch:
    send_batch_size: 1000
    timeout: 200ms

2020-06-17 10:05:26 INFO  Main - Expected number of spans 100000, actual 100000 stored in 39s
2020-06-17 10:24:00 INFO  Main - Expected number of spans 100000, actual 100000 stored in 39s
2020-06-17 10:27:46 INFO  Main - Expected number of spans 300000, actual 300000 stored in 105s
2020-06-17 10:31:17 INFO  Main - Expected number of spans 300000, actual 300000 stored in 96s
2020-06-17 10:37:41 INFO  Main - Expected number of spans 300000, actual 300000 stored in 111s
2020-06-17 10:55:03 INFO  Main - Expected number of spans 300000, actual 300000 stored in 115s
2020-06-17 11:04:25 INFO  Main - Expected number of spans 300000, actual 300000 stored in 164s
2020-06-17 11:10:55 INFO  Main - Expected number of spans 300000, actual 300000 stored in 121s

Jaeger all-in-one --es.bulk.size=10000000000000, --collector.queue-size=300000

2020-06-17 11:20:35 INFO  Main - Expected number of spans 300000, actual 300000 stored in 181s
2020-06-17 11:24:38 INFO  Main - Expected number of spans 300000, actual 300000 stored in 166s
2020-06-17 11:29:54 INFO  Main - Expected number of spans 300000, actual 300000 stored in 172s
2020-06-17 11:49:06 INFO  Main - Expected number of spans 300000, actual 300000 stored in 165s

codecov · 2020-06-18T16:48:47Z

Codecov Report

Merging #2295 into master will decrease coverage by 0.56%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2295      +/-   ##
==========================================
- Coverage   96.21%   95.64%   -0.57%     
==========================================
  Files         203      205       +2     
  Lines       10291    10529     +238     
==========================================
+ Hits         9901    10070     +169     
- Misses        334      391      +57     
- Partials       56       68      +12

Impacted Files	Coverage Δ
plugin/storage/integration/trace_compare.go	`36.58% <ø> (ø)`
plugin/storage/integration/integration.go	`80.86% <92.85%> (ø)`
plugin/storage/es/factory.go	`100.00% <100.00%> (ø)`
cmd/query/app/server.go	`90.90% <0.00%> (-2.28%)`	⬇️
cmd/flags/admin.go	`77.77% <0.00%> (-1.59%)`	⬇️
plugin/storage/badger/spanstore/reader.go	`96.79% <0.00%> (ø)`

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 7c9e1d3...369d1af. Read the comment docs.

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/client.go

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/es6client.go

cmd/opentelemetry/app/exporter/elasticsearchexporter/estranslator/modeltranslator.go

cmd/opentelemetry/app/exporter/elasticsearchexporter/estranslator/modeltranslator_test.go

objectiser · 2020-06-19T13:48:32Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/integration_test.go

+}
+
+func (s storageWrapper) WriteSpan(span *model.Span) error {
+	converter := dbmodel.FromDomain{}


Does this need to be created for each span?

This is a workaround, the purpose of the storage test is the correctness. Although It would be better to convert from jaeger to OTEL and then use the remain interface method.

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanstore.go

objectiser · 2020-06-19T14:08:26Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanstore.go

+// WriteTraces writes traces to the storage
+func (w *EsSpanWriter) WriteTraces(_ context.Context, traces pdata.Traces) (int, error) {
+	atomic.AddInt64(&counter, int64(traces.SpanCount()))
+	fmt.Printf("counter: %d\n", counter)


I will remove the prints once it's ready. Just ignore them for now

objectiser · 2020-06-19T14:14:06Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanstore.go

+			return len(spans), err
+		}
+		indexName := w.spanIndexName.get(model.EpochMicrosecondsAsTime(span.StartTime))
+		operationsToSpanIndices = append(operationsToSpanIndices, i)


Why is this repeated twice? on line 150

The array holds a mapping from bulk operation to a span index number.

It will be used to construct the retry error of the bulk operation fails. It's there twice because service operation might also fail and it has a different bulk ID.

objectiser · 2020-06-19T14:26:18Z

pkg/es/config/config.go

@@ -366,3 +376,23 @@ func loadToken(path string) (string, error) {
 	}
 	return strings.TrimRight(string(b), "\r\n"), nil
 }
+
+// LoadTagsFromFile loads tags from a file
+func LoadTagsFromFile(filePath string) ([]string, error) {


At the moment I don't think it is possible to specify a list of tags? Only via the file? If so, possibly this is something we may want to add to the OTel config for the exporter?

We could consider doing that, perhaps in a separate issue.

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/es7client.go

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/client.go

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/es7client.go

jpkrohling · 2020-06-22T12:17:39Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/es7client.go

+	}
+	defer response.Body.Close()
+	if response.StatusCode >= 400 {
+		return nil, fmt.Errorf("bulk request failed with code %d", response.StatusCode)


Are there cases where a retry could be used? Like, a 502 or 503 could probably be retried in a few milliseconds.

for the retries we want to use a queued retry processor.

What about 3xx-class responses?

ES client should retry with the provided URL.

cmd/opentelemetry/app/exporter/elasticsearchexporter/estranslator/modeltranslator.go

cmd/opentelemetry/app/exporter/elasticsearchexporter/factory_test.go

jpkrohling · 2020-06-22T12:49:29Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanstore.go

+		serviceIndexName: newIndexNameProvider("jaeger-service", params.IndexPrefix, params.UseReadWriteAliases),
+		translator:       estranslator.NewTranslator(params.Tags.AllAsFields, tagsKeysAsFields, params.GetTagDotReplacement()),
+		serviceCache: cache.NewLRUWithOptions(
+			100000,


I think this deserves a comment :) Where did this default come from? What are the implications? If there are more than 100_000 services, what happens (other than the chief architect being forcefully admitted to the psych ward)?

Where did this default come from?

The default is copy-paste from the old ES impl.

If there are more than 100_000 services, what happens (

Based on the name of the cache, the oldest used item will be evicted. I don't expect anybody running more than 100k services ;)

Murphy's law dictates that someone will run into this limit. For that pool soul's sake, could you add a log message? These kinds of limitations/bugs are the hardest to sort out...

The size of the cache does not have affect on the correctness of the implementation. 100k is used in the current implementation without any issues.

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanstore.go

jpkrohling · 2020-06-22T12:55:08Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanstore.go

+	for i, d := range blk.Items {
+		// ... so for any HTTP status above 201 ...
+		//
+		if d.Index.Status > 201 {


Is this what the ES semantics dictate? Technically, everything in the 2xx range is "success". Notoriously, 204 is used in a few systems out there instead of 200, where a response isn't really required.

This is what all the tutorials and code examples from the official repository do, so I guess it should be like this.

pkg/es/config/config.go

.travis.yml

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/client.go

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/elasticsearch_ping.go

yurishkuro · 2020-06-23T02:20:19Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/elasticsearch_ping.go

+	if err != nil {
+		return 0, err
+	}
+	esVersion, err := strconv.Atoi(string(pingResponse.Version.Number[0]))


Is this looking at the first char? Meaning it will break on v10.

Yes, there are other parts of the code where the new version will fail

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/es7client.go

yurishkuro · 2020-06-23T02:57:06Z

pkg/es/config/config.go

+	}
+	var transport http.RoundTripper
+	httpTransport := &http.Transport{
+		Proxy: http.ProxyFromEnvironment,


I prefer creating objects only once. Does this Proxy setting not apply to L313?

This is legacy code from this file, I will not refactor it in this PR. I have just moved it to a separate func so I can reuse it in other parts. The implementation indeed looks like that it could be refactored.

yurishkuro · 2020-06-23T02:59:52Z

pkg/es/config/config.go

+		// #nosec G402
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: c.TLS.SkipHostVerify},
+	}
+	if c.TLS.CAPath != "" {


this feels a bit convoluted & duplicated - why wouldn't this path be covered by L308?

see the previous comment #2295 (comment)

plugin/storage/integration/grpc_test.go

pavolloffay · 2020-06-29T09:47:14Z

I cannot rebase and merge until #2319 is resolved as this PR needs changes in the core.

pavolloffay · 2020-06-29T12:53:26Z

The code is in the final state. The missing item from TODO:

ES exporter should probably accept its own params struct and not config.Configuration

it's not critical. The query configuration options are not exposed in the OTEL config. I will consider doing refactoring in a follow up PR.

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanstore.go

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

* Reimplement OTEL elasticsearch exporter Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix typo in comment Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay added storage/elasticsearch area/otel labels Jun 18, 2020

pavolloffay requested review from jpkrohling and objectiser June 18, 2020 15:10

pavolloffay requested a review from a team as a code owner June 18, 2020 15:10

pavolloffay force-pushed the otel-es-new branch from 24e33fe to 3b74c4b Compare June 18, 2020 15:11

objectiser reviewed Jun 19, 2020

View reviewed changes

objectiser approved these changes Jun 22, 2020

View reviewed changes

cmd/opentelemetry/app/exporter/elasticsearchexporter/esclient/es7client.go Outdated Show resolved Hide resolved

jpkrohling reviewed Jun 22, 2020

View reviewed changes

pavolloffay mentioned this pull request Jun 22, 2020

Expose ES tag keys directly in the OTEL config file #2301

Closed

yurishkuro reviewed Jun 23, 2020

View reviewed changes

objectiser reviewed Jun 29, 2020

View reviewed changes

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanstore.go Outdated Show resolved Hide resolved

pavolloffay mentioned this pull request Jun 30, 2020

Duplicate tag span.kind=server #2321

Closed

pavolloffay added 2 commits July 1, 2020 13:26

Reimplement OTEL elasticsearch exporter

dfa379e

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

Fix typo in comment

369d1af

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

pavolloffay force-pushed the otel-es-new branch from 1ee9233 to 369d1af Compare July 1, 2020 11:27

pavolloffay merged commit 00b6e96 into jaegertracing:master Jul 1, 2020

This was referenced Jul 4, 2020

Test failing -- TestElasticSearchSuite/TestSparkDependenciesES jaegertracing/jaeger-operator#1113

Closed

fix null pointer in jaeger-spark-dependencies #2327

Merged

joe-elliott mentioned this pull request Aug 26, 2020

Allow configuration of ES tag keys directly through config/cli #2417

Merged

pavolloffay mentioned this pull request Aug 20, 2021

Add size in bytes to batch processor open-telemetry/opentelemetry-collector-contrib#4761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add native OTEL ES exporter #2295

Add native OTEL ES exporter #2295

pavolloffay commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading

objectiser Jun 19, 2020

pavolloffay Jun 22, 2020

objectiser Jun 19, 2020

pavolloffay Jun 22, 2020

objectiser Jun 19, 2020

pavolloffay Jun 22, 2020

objectiser Jun 19, 2020

pavolloffay Jun 22, 2020

objectiser Jun 22, 2020

pavolloffay Jun 22, 2020

jpkrohling Jun 22, 2020

pavolloffay Jun 22, 2020

jpkrohling Jun 23, 2020

pavolloffay Jun 23, 2020

jpkrohling Jun 22, 2020

pavolloffay Jun 22, 2020

jpkrohling Jun 23, 2020

pavolloffay Jun 23, 2020

jpkrohling Jun 22, 2020

pavolloffay Jun 23, 2020

yurishkuro Jun 23, 2020

pavolloffay Jun 23, 2020

yurishkuro Jun 23, 2020

pavolloffay Jun 23, 2020

yurishkuro Jun 23, 2020

pavolloffay Jun 23, 2020

pavolloffay commented Jun 29, 2020

pavolloffay commented Jun 29, 2020

Add native OTEL ES exporter #2295

Add native OTEL ES exporter #2295

Conversation

pavolloffay commented Jun 18, 2020 • edited Loading

Performance

codecov bot commented Jun 18, 2020 • edited Loading

Codecov Report

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

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

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

pavolloffay commented Jun 29, 2020

pavolloffay commented Jun 29, 2020

pavolloffay commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading