Add new Elasticsearch reader implementation #2364

pavolloffay · 2020-07-31T13:35:26Z

This PR adds new Elasticsearch reader implementation that uses https://github.com/elastic/go-elasticsearch which is the client that ES OTEL exporter uses and we would like to use it for all interactions with ES.

Notable changes:

Uses https://github.com/elastic/go-elasticsearch client
The client exposes low-level API and it does not expose any query/response objects. The consumer code has to define all objects.
The query/response objects defined in this impl are not general-purpose and cover only use cases that our read API uses.
This new implementation uses similar queries and logic as the current reader https://github.com/jaegertracing/jaeger/tree/master/plugin/storage/es/spanstore

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

objectiser

Only part way through the PR, will look at the rest later - but one question I wanted to ask was about the location the ES client code.

Previously it was under the otel exporter because it was only handling ES writer - but now it also includes the query/reader side, having it reside under the exporter hierarchy is not so meaningful.

If we were to move it to a more independent location, would this cause issues with dependencies from the exporter? Or is there a suitable location that would be ok to use from exporter and query service?

cmd/opentelemetry/app/exporter/elasticsearchexporter/dependencystore/dependency_store.go

pavolloffay · 2020-08-04T12:50:42Z

If we were to move it to a more independent location, would this cause issues with dependencies from the exporter? Or is there a suitable location that would be ok to use from exporter and query service?

The new reader has to reside in the same go module as the esclient package which is at the moment the Jaeger OTEL go module.

We should move the esclient to Jaeger core module with the reader. I would rather do that once this is merged to keep the changes minimal as this is already too large :/.

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

codecov · 2020-08-05T07:53:18Z

Codecov Report

Merging #2364 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2364      +/-   ##
==========================================
- Coverage   95.61%   95.60%   -0.02%     
==========================================
  Files         206      206              
  Lines       10542    10548       +6     
==========================================
+ Hits        10080    10084       +4     
- Misses        394      396       +2     
  Partials       68       68

Impacted Files	Coverage Δ
...lugin/sampling/strategystore/adaptive/processor.go	`99.20% <0.00%> (-0.80%)`	⬇️
plugin/storage/integration/integration.go	`80.38% <0.00%> (-0.48%)`	⬇️
plugin/storage/kafka/options.go	`94.05% <0.00%> (+0.37%)`	⬆️
cmd/flags/admin.go	`79.36% <0.00%> (+1.58%)`	⬆️

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 9529bee...c379098. Read the comment docs.

objectiser

A few more comments - will eventually complete the review :)

objectiser · 2020-08-05T15:17:21Z

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

+	"io"
+)
+
+const multiSearchHeaderFormat = `{"ignore_unavailable": "true"}` + "\n"


Could you add a comment to explain how this value relates to "HeaderFormat"?

objectiser · 2020-08-05T15:29:50Z

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

@@ -115,57 +113,55 @@ func (s *IntegrationTest) esCleanUp(allTagsAsFields bool) error {
 }

 func (s *IntegrationTest) initSpanstore(allTagsAsFields bool) error {
-	bp, _ := s.client.BulkProcessor().BulkActions(1).FlushInterval(time.Nanosecond).Do(context.Background())
-	s.bulkProcessor = bp


Assume the bulk processor is being removed as it will now be supported via the otel batch processor?

cmd/opentelemetry/app/exporter/elasticsearchexporter/dependencystore/dependency_store.go

objectiser

LGTM - just the few comments here and above.

cmd/opentelemetry/app/exporter/elasticsearchexporter/spanreader/validate_query.go

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

pavolloffay · 2020-08-12T14:53:04Z

@objectiser PR updated

idohalevi · 2020-10-07T19:44:50Z

@pavolloffay I've noticed that since this PR the query to ES, when tags included, changed from:

"bool": { "must": { "match": { "tag.error": { "query": "true" } } } }

To:

"must": { "regexp": { "tag.error": { "value": "true" } } }

Any reason for this? I get 500 when I'm trying to search any tag that is not mapped to a keyword

pavolloffay · 2020-10-08T07:09:17Z

It might be a bug, could you please open an issue with some reproducer?

idohalevi · 2020-10-08T08:09:03Z

@pavolloffay sure #2540

pavolloffay requested a review from a team as a code owner July 31, 2020 13:35

pavolloffay requested a review from jpkrohling July 31, 2020 13:35

pavolloffay changed the title ~~Add new Elasticsearch reader implementation~~ WIP: Add new Elasticsearch reader implementation Jul 31, 2020

pavolloffay changed the title ~~WIP: Add new Elasticsearch reader implementation~~ Add new Elasticsearch reader implementation Aug 3, 2020

pavolloffay added the storage/elasticsearch label Aug 4, 2020

Add new Elasticsearch reader backed by go-elasticsearch

ff01edd

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

pavolloffay force-pushed the es-new-reader branch from 9b66cf9 to ff01edd Compare August 4, 2020 07:28

pavolloffay added 2 commits August 4, 2020 09:42

add dash to index prefix in dep reader

7202801

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

Use separate config object from ES clients

01a902c

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

objectiser mentioned this pull request Aug 4, 2020

Jaeger query doesn't propagate auth token on dependency endpoint #2369

Closed

objectiser reviewed Aug 4, 2020

View reviewed changes

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

pavolloffay added 2 commits August 4, 2020 14:58

Fix typo

a70b610

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

Fix track results

2660595

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

pavolloffay closed this Aug 5, 2020

pavolloffay reopened this Aug 5, 2020

objectiser reviewed Aug 5, 2020

View reviewed changes

objectiser reviewed Aug 7, 2020

View reviewed changes

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

Fix review comments

c379098

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

objectiser approved these changes Aug 12, 2020

View reviewed changes

pavolloffay merged commit 2b5fe11 into jaegertracing:master Aug 19, 2020

idohalevi mentioned this pull request Oct 8, 2020

Search traces with non-string tags #2540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Elasticsearch reader implementation #2364

Add new Elasticsearch reader implementation #2364

pavolloffay commented Jul 31, 2020 •

edited

Loading

objectiser left a comment •

edited

Loading

pavolloffay commented Aug 4, 2020

codecov bot commented Aug 5, 2020 •

edited

Loading

objectiser left a comment

objectiser Aug 5, 2020

objectiser Aug 5, 2020

pavolloffay Aug 12, 2020

objectiser left a comment

pavolloffay commented Aug 12, 2020

idohalevi commented Oct 7, 2020

pavolloffay commented Oct 8, 2020

idohalevi commented Oct 8, 2020

Add new Elasticsearch reader implementation #2364

Add new Elasticsearch reader implementation #2364

Conversation

pavolloffay commented Jul 31, 2020 • edited Loading

objectiser left a comment • edited Loading

Choose a reason for hiding this comment

pavolloffay commented Aug 4, 2020

codecov bot commented Aug 5, 2020 • edited Loading

Codecov Report

objectiser left a comment

Choose a reason for hiding this comment

objectiser Aug 5, 2020

Choose a reason for hiding this comment

objectiser Aug 5, 2020

Choose a reason for hiding this comment

pavolloffay Aug 12, 2020

Choose a reason for hiding this comment

objectiser left a comment

Choose a reason for hiding this comment

pavolloffay commented Aug 12, 2020

idohalevi commented Oct 7, 2020

pavolloffay commented Oct 8, 2020

idohalevi commented Oct 8, 2020

pavolloffay commented Jul 31, 2020 •

edited

Loading

objectiser left a comment •

edited

Loading

codecov bot commented Aug 5, 2020 •

edited

Loading