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.

Sign up for GitHub

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

Jump to bottom

Adds support for Elasticsearch 7.x #2398

Merged
merged 10 commits into from
May 3, 2019
Merged

Adds support for Elasticsearch 7.x #2398

merged 10 commits into from
May 3, 2019

Conversation

codefromthecrypt
Copy link
Member

There are multiple issues to resolve, not just the colon banning.

See https://www.elastic.co/guide/en/elasticsearch/reference/7.x/breaking-changes-7.0.html
See #2219

@codefromthecrypt codefromthecrypt added the elasticsearch Elasticsearch storage component label Feb 18, 2019
@codefromthecrypt
Copy link
Member Author

the thing breaks on lack of default mapping. We have some things there, so we have to be careful how to address this. I'm putting this change down because I didn't anticipate this being more than just the colon problem.

@codefromthecrypt codefromthecrypt mentioned this pull request Feb 18, 2019
Merged
@making
Copy link
Member

making commented Apr 11, 2019

Elasticsearch 7.0 has been released.
Is there any workaround to change the index pattern zipkin uses?

@codefromthecrypt
Copy link
Member Author

@making it is not just an issue of the index naming convention. that's a very small matter actually. they've made significant changes to the index template itself

@making
Copy link
Member

making commented Apr 11, 2019

@adriancole so isn't ES 7.0 available until this issue is fixed?

@codefromthecrypt
Copy link
Member Author

@making this pull request needs to pass :) the index template needs to work and it doesn't on ES 7, so that's the main issue. When done debugging other things, I'll throw some time on this. It is good to know that ES 7 is final now.

@codefromthecrypt
Copy link
Member Author

there's actually another significant issue which is the revlock on the dependency aggregation job. To have dependencies link on ES 7 probably implies breaking ES 2.x openzipkin/zipkin-dependencies#124 cc @openzipkin/elasticsearch

@making
Copy link
Member

making commented Apr 11, 2019

Do you continue to support ES 2x?

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Apr 11, 2019 •
edited
Loading

@making we've not yet unsupported it, so yeah we run tests on it on every build. I don't think we would want to un-support it at least until lens is default as otherwise people will revlock into the old UI.

amazon still lets people spin up 1.5! https://aws.amazon.com/elasticsearch-service/faqs/

@codefromthecrypt
Copy link
Member Author

I found the two blockers on zipkin-dependencies. holding until they are sorted as it will not make sense to proceed here if we can't aggregate dependencies.

elastic/elasticsearch-hadoop#1277
elastic/elasticsearch-hadoop#1276

@codefromthecrypt
Copy link
Member Author

@gquintana until tests pass there's no point in doing work that feigns ES 7 support. main thing needed here is to finish the drift. there was a dramatic change in index templates introduced in ES 7, iotw changing the pattern doesn't actually make things work. this PR starts the process and if you want you can finish this change if I don't finish it first.

right now, we are having a workshop, so I won't be working on this for the next few days. separately ES have no working version of elasticsearch-hadoop anyway as noted by issues not yet acked by ES folks
elastic/elasticsearch-hadoop#1276
elastic/elasticsearch-hadoop#1277

codefromthecrypt
codefromthecrypt commented Apr 17, 2019
View reviewed changes
zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/ElasticsearchSpanConsumer.java
final boolean searchEnabled;
final DelayLimiter<AutocompleteContext> delayLimiter;

ElasticsearchSpanConsumer(ElasticsearchStorage es) {
this.es = es;
this.autocompleteKeys = new LinkedHashSet<>(es.autocompleteKeys());
this.indexNameFormatter = es.indexNameFormatter();
this.indexTypeDelimiter = es.indexTypeDelimiter();
Copy link
Member Author

Choose a reason for hiding this comment

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

super note: literally this is not enough! let's please defocus from this being the single issue blocking ES 7 support

@codefromthecrypt
Copy link
Member Author

when this change is done, nagging on this issue will be welcome again :) #2513

hopefully, that's not longer than a week.

This was referenced Apr 27, 2019
Closed
Closed
WIP: attempts support of Elasticsearch 7.x
923a04d 
There are multiple issues to resolve, not just the colon banning.
@codefromthecrypt
Copy link
Member Author

current status. no longer any errors installing the index template on ES 7. However, all the query tests are broke

@codefromthecrypt
Copy link
Member Author

a couple more mapping problems..

java.lang.IllegalStateException: {"took":676,"errors":true,"items":[{"index":{"_index":"gettagsandvalues-span-2019-05-03","_type":"span","_id":"rQgAfGoBmB-cc_Lhh4f3","status":500,"error":{"type":"mapper_exception","reason":"Can't update attribute for type [span.tags.enabled] in index mapping"}}},{"index":{"_index":"gettagsandvalues-autocomplete-2019-05-03","_type":"autocomplete","_id":"http.host=host1","status":400,"error":{"type":"illegal_argument_exception","reason":"Rejecting mapping update to [gettagsandvalues-autocomplete-2019-05-03] as the final mapping would have more than 1 type: [_doc, autocomplete]"}}}]}

@codefromthecrypt
Copy link
Member Author

@making I tested this branch and it now works (eventhough the spark job is still messed up for reasons noted multiple times). Can you or someone else verify?

$ TAG=elasticsearch7-SNAPSHOT
$ curl -sSL https://jitpack.io/com/github/apache/incubator-zipkin/zipkin-server/${TAG}/zipkin-server-${TAG}-exec.jar > zipkin.jar
$ STORAGE_TYPE=elasticsearch java -jar zipkin.jar

@making
Copy link
Member

making commented May 3, 2019

@adriancole Thanks! I'll try it on my env.

@codefromthecrypt
Copy link
Member Author

actually this week is still golden week in Japan. @gquintana care to give a swing? ^^ for a temporary download location

@codefromthecrypt
Copy link
Member Author

hah wires crossed @making thanks for not being on holiday :)

@making
Copy link
Member

making commented May 3, 2019 •
edited
Loading

No problem, it's a golden development week :)

I confirmed that Zipkin started to create a zipkin-span-{yyyy}-{mm}-{dd} index.

image

But it's weird to see only one service sends traces ... (There should be 3 or 4 services.)

image

image

@codefromthecrypt
Copy link
Member Author

Thanks for verifying, @making the service change could be unrelated recent change landed in the next pending release https://github.com/apache/incubator-zipkin/releases/tag/v2.13.0

@codefromthecrypt
Copy link
Member Author

I'll merge this. this commit doesn't remove support for ES 2.x or anything just adds 7.x. More follow-ups will happen mostly in https://github.com/openzipkin/zipkin-dependencies/ and this change will go out in the next server release after 2.13 (likely will be named 2.14)

@codefromthecrypt codefromthecrypt merged commit 762a795 into master May 3, 2019
@codefromthecrypt codefromthecrypt deleted the elasticsearch7 branch May 3, 2019 11:08
@codefromthecrypt codefromthecrypt changed the title WIP: attempts support of Elasticsearch 7.x Adds support for Elasticsearch 7.x May 3, 2019
@codefromthecrypt
Copy link
Member Author

the above jitpack curl command should also work for master now (TAG=master-SNAPSHOT)

@codefromthecrypt
Copy link
Member Author

ps I just tried with brave-webmvc-example just to verify service names are ok. I think it is ok.

Screen Shot 2019-05-03 at 7 19 10 PM

@chefky
Copy link

chefky commented May 6, 2019 via email

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented May 6, 2019 via email

@chefky
Copy link

chefky commented May 6, 2019 via email

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented May 6, 2019 via email

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented May 6, 2019 •
edited
Loading

@chefky it is near impossible for us to warranty anything custom with elasticsearch especially as versions change. keeping that in mind, I spent an hour trying to figure out the magic incantation and it "seems to work" on my laptop. If you need more support on elasticsearch, probably best to ask them as they know the nuance here. anyway with all disclosures aside.. https://gist.github.com/adriancole/1af1259102e7a2da1b3c9103565165d7

@pete-woods
Copy link

Thanks for fixing this so quickly, folks. Looking forward to 2.14!

@making
Copy link
Member

making commented May 10, 2019

my update:

But it's weird to see only one service sends traces ... (There should be 3 or 4 services.)

All services needed to be restarted to get all traces again. It works fine now :)

@nopeno
Copy link

nopeno commented Jul 31, 2019

when i installed elasticsearch 7.2.0, i got:

failed to put template [zipkin:span_template]
java.lang.IllegalArgumentException: Setting index.mapper.dynamic was removed after version 6.0.0
at org.elasticsearch.index.mapper.MapperService.(MapperService.java:161) ~[elasticsearch-7.2.0.jar:7.2.0]
at org.elasticsearch.index.IndexService.(IndexService.java:175) ~[elasticsearch-7.2.0.jar:7.2.0]
at org.elasticsearch.index.IndexModule.newIndexService(IndexModule.java:399) ~[elasticsearch-7.2.0.jar:7.

@shakuzen
Copy link
Member

@nopeno it would be better to chat on Gitter, since this is an already merged pull request. Please make sure you're using the latest version of the Zipkin Server, and if you're still having issues, let us know at https://gitter.im/openzipkin/zipkin

abesto pushed a commit to abesto/zipkin that referenced this pull request Sep 10, 2019
@adriancole @abesto
Supports Elasticsearch 7.x (openzipkin#2398)
8fd4c4d 
Fixes openzipkin#2219
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
elasticsearch Elasticsearch storage component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@codefromthecrypt @making @chefky @pete-woods @nopeno @shakuzen