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

Use new ES reader implementation in OTEL #2441

Merged
merged 4 commits into from
Sep 3, 2020

Conversation

pavolloffay
Copy link
Member

Signed-off-by: Pavol Loffay [email protected]

This PR switches to use new ES reader implementation in OTEL all-in-one distribution.

Notable changes:

  • the OTEL ES implements storage factory
  • added parsing of response error
  • added support for archive storage writer
  • fixed error for reading spans from archive storage (search after has to be with sort)

@pavolloffay
Copy link
Member Author

@joe-elliott would you like to have a look at this one too?

Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2441      +/-   ##
==========================================
+ Coverage   95.56%   95.58%   +0.01%     
==========================================
  Files         208      208              
  Lines       10679    10679              
==========================================
+ Hits        10205    10207       +2     
+ Misses        401      400       -1     
+ Partials       73       72       -1     
Impacted Files Coverage Δ
plugin/storage/badger/spanstore/reader.go 96.79% <0.00%> (ø)
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.79%) ⬆️

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 8630a31...025d422. Read the comment docs.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

It took me awhile to get my head around these changes. A few small questions asked, but otherwise looks good.

Primarily this seems to be the addition of a storage.Factory shim to make use of the elasticchsearchexporter and esspanreader packages in the otel version of the all-in-one. This required the addition of support for archive functionality which is why it was added as well.

Aggs map[string]AggregationResponse `json:"aggregations,omitempty"`
Hits Hits `json:"hits"`
Aggs map[string]AggregationResponse `json:"aggregations,omitempty"`
Error *SearchResponseError `json:"error,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

not seeing this field mentioned in the docs, but i'm probably misunderstanding something.
https://www.elastic.co/guide/en/elasticsearch/reference/current/search-search.html

Copy link
Member Author

Choose a reason for hiding this comment

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

There is an error, I will test again if that is the case for all ES versions

Copy link
Member Author

Choose a reason for hiding this comment

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

ES

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" -e "xpack.security.enabled=false" --name=elasticsearch  docker.elastic.co/elasticsearch/elasticsearch:5.6.10

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

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:7.8.1

All returned the following error:

curl -ivX POST -H "Content-Type: application/json" http://localhost:9200/jaeger/_search\?pretty -d @query.json
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying 127.0.0.1:9200...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 9200 (#0)
> POST /jaeger/_search?pretty HTTP/1.1
> Host: localhost:9200
> User-Agent: curl/7.66.0
> Accept: */*
> Content-Type: application/json
> Content-Length: 160
> 
* upload completely sent off: 160 out of 160 bytes
* Mark bundle as not supporting multiuse
< HTTP/1.1 400 Bad Request
HTTP/1.1 400 Bad Request
< content-type: application/json; charset=UTF-8
content-type: application/json; charset=UTF-8
< content-length: 378
content-length: 378

< 
{
  "error" : {
    "root_cause" : [
      {
        "type" : "parsing_exception",
        "reason" : "Unknown key for a VALUE_STRING in [ignore_unavailable].",
        "line" : 1,
        "col" : 24
      }
    ],
    "type" : "parsing_exception",
    "reason" : "Unknown key for a VALUE_STRING in [ignore_unavailable].",
    "line" : 1,
    "col" : 24
  },
  "status" : 400
}
* Connection #0 to host localhost left intact

query.json (multi search query)

{"ignore_unavailable": "true"}
{"query":{"term":{"traceID":{"value":"649f0a8fe3184a3f"}}},"size":10000,"terminate_after":10000,"search_after":[1598699865301838]}

@@ -0,0 +1,51 @@
// Copyright (c) 2020 The Jaeger Authors.
Copy link
Member

Choose a reason for hiding this comment

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

should this be consolidated with the indexNameProvider in app/internal/reader/es/esspanreader ? Since these index names would have to coordinate for reading and writing to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can try to do that in a separate PR. Note that one returns a list of indices and the other one a single index.

@pavolloffay pavolloffay merged commit 043d00c into jaegertracing:master Sep 3, 2020
albertteoh pushed a commit to albertteoh/jaeger that referenced this pull request Sep 3, 2020
* Use new ES reader implementation in OTEL

Signed-off-by: Pavol Loffay <[email protected]>

* fmt

Signed-off-by: Pavol Loffay <[email protected]>

* Fix ITest

Signed-off-by: Pavol Loffay <[email protected]>

* fixes after rebase

Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: albertteoh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants