Composable index templates for ES 7.8 #3185

ccharnkij · 2020-08-22T21:40:38Z

Description

For #3138, this PR lets Zipkin uses composable index templates for ES >= 7.8. Also, a new env variable is added for index template priority.

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Unit tests are added for the features. The unit tests passed when ran locally. Also, I have done E2E tests using both ES >=7.8 and ES < 7.8 using the brave sample project.

Checklist:

My code follows the style guidelines of this project
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

codefromthecrypt

This looks very good!

I would add something in the ITElasticsearch7 similar to ITEnsureSchema in cassandra, that sets up something that this priority would ensure doesn't mess up. Ex start ES docker and add the template prior to zipkin doing so. then make sure the things that failed before doesn't.

This way, we can automatically ensure something like @benqs comment doesn't hit us on 7.9 openzipkin/zipkin-support#23 (comment)

Also add a test for version 7.9 is same behavior as sometimes eyes cross on expressions. This happened to us in cassandra before.

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/VersionSpecificTemplates.java

codefromthecrypt · 2020-08-22T23:52:06Z

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/VersionSpecificTemplates.java

@@ -141,6 +167,8 @@ String spanIndexTemplate(float version) {
        + "      \"_q\": " + KEYWORD + "\n"
        + "    }\n")
        + "  }\n"
+        + indexTemplateClosing(version)


squash this and below into endTemplate as it is distracting to add template priority here when sometimes there's no template anyway.

zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/ElasticsearchStorageTest.java

codefromthecrypt · 2020-08-22T23:54:16Z

...-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/VersionSpecificTemplatesTest.java

+      .withFailMessage("Starting at v7.x, we delimit index and type with hyphen")
+      .contains("\"index_patterns\": \"zipkin-autocomplete-*\"");
+    assertThat(template.span())
+      .contains("\"template\": {\n")


nice. I suppose at some point we should switch this assert to json path, but works for me.

ccharnkij · 2020-08-24T07:38:07Z

@adriancole Please review again. I made begin/endTemplate and added commented on tests. I also added to ITElasticsearch7 as you suggested, I created a 'catch-all' template with pattern '*' and priority 0 prior to the tests, and the tests should proceed as normal.

I know that there are license fail checks, but since I didn't add any new file, I wasn't sure about updating the license header.

Also, I wasn't sure what you mean about tests for version 7.9. The code/tests that I added should apply to version 7.8 and above. Unless I'm missing something, please let me know.

codefromthecrypt · 2020-08-24T09:11:21Z

FYI command to update license is here https://github.com/openzipkin/zipkin/blob/master/.github/CONTRIBUTING.md

codefromthecrypt · 2020-08-24T09:12:07Z

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/ElasticsearchStorage.java

@@ -88,7 +88,8 @@ public static Builder newBuilder(LazyHttpClient lazyHttpClient) {
      .flushOnWrites(false)
      .autocompleteKeys(Collections.emptyList())
      .autocompleteTtl((int) TimeUnit.HOURS.toMillis(1))
-      .autocompleteCardinality(5 * 4000); // Ex. 5 site tags with cardinality 4000 each
+      .autocompleteCardinality(5 * 4000)
+      .templatePriority("0"); // Ex. 5 site tags with cardinality 4000 each


this comment doesn't belong. probably you want to say // highest priority

codefromthecrypt · 2020-08-24T09:12:55Z

zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/ElasticsearchStorageTest.java

+   */
+  @Test void check_create_composable_indexTemplate_resourcePath() throws Exception {
+    server.enqueue(AggregatedHttpResponse.of(
+      HttpStatus.OK, MediaType.JSON_UTF_8, "{\"version\":{\"number\":\"7.8.0\"}}"));


for example, eaisiest way is to change this to 7.9

codefromthecrypt

thanks for the revision. Ideally the composite test is not implicit, rather a separate test. if the version is <7.8 you can use https://junit.org/junit5/docs/current/api/org.junit.jupiter.api/org/junit/jupiter/api/Assumptions.html#assumeTrue(java.util.function.BooleanSupplier)

codefromthecrypt · 2020-08-24T09:13:27Z

.../elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITElasticsearchStorageV7.java

@@ -13,8 +13,14 @@
 */
 package zipkin2.elasticsearch.integration;

+import com.linecorp.armeria.common.*;


if using square formatter it will expand wildcard imports

codefromthecrypt · 2020-08-24T09:13:40Z

...ticsearch/src/test/java/zipkin2/elasticsearch/integration/ElasticsearchStorageExtension.java

@@ -116,6 +116,7 @@ Builder computeStorageBuilder() {
    WebClient client = builder.build();
    return ElasticsearchStorage.newBuilder(() -> client)
      .index("zipkin-test")
+      .templatePriority("10")


maybe revert this as it effects all tests.. that or comment why.

codefromthecrypt · 2020-08-24T09:15:12Z

.../elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITElasticsearchStorageV7.java

+   * ensure that the index templates created during tests with higher priority function as
+   * designed. Only applicable for ES >= 7.8
+   */
+  @BeforeAll void setUpCatchAllTemplate() throws IOException {


should be a separate test instead of a before for everything.

ccharnkij · 2020-08-28T06:45:00Z

@adriancole 3rd time's a charm?

codefromthecrypt

Thanks I think you did the hard work. Main thing is to isolate the catch-all to a single test and also re-scope it so that it would only ever invoke on ES 7.8+

codefromthecrypt · 2020-08-28T07:16:57Z

...ge/elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITElasticsearchStorage.java

@@ -114,4 +114,12 @@
      storage.clear();
    }
  }
+
+  @Nested
+  class ITEnsureIndexTemplate extends zipkin2.elasticsearch.integration.ITEnsureIndexTemplate {


I think you can move this to the ITEs7 class?

codefromthecrypt · 2020-08-28T07:18:03Z

...age/elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITEnsureIndexTemplate.java

+   */
+  protected abstract ElasticsearchStorage.Builder newStorageBuilder(TestInfo testInfo);
+
+  @BeforeAll void setUpCatchAllTemplate(TestInfo testInfo) throws IOException {


with this pinned to the ES 7 IT, you don't need to check version as our image is already this I think right? In this case you can do this in the test method itself

codefromthecrypt · 2020-08-28T07:19:41Z

...age/elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITEnsureIndexTemplate.java

+  /**
+   * Delete "catch-all" index template so it does not interfere with any other test
+   */
+  @AfterAll void deleteCatchAllTemplate() throws IOException {


this should be a "finally" of the catchAllTemplateTest as it would only need to be tested once anyway

codefromthecrypt · 2020-08-28T07:20:57Z

...age/elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITEnsureIndexTemplate.java

+
+    storage.spanConsumer().accept(spans).execute();
+
+    assertThat(storage.traces().getTraces(traceIds).execute())


verify the catch-all template works? you might want to make something more interesting so that you can use ES HTTP query on it. ex this one https://gist.github.com/adriancole/1af1259102e7a2da1b3c9103565165d7

ccharnkij · 2020-08-29T10:40:26Z

@adriancole like that?

codefromthecrypt

Almost there! let me know if you are out of steam, but otherwise it would be great if you can finish this. Thanks again

codefromthecrypt · 2020-08-30T00:22:24Z

.../elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITElasticsearchStorageV7.java

+
+  @Nested
+  class ITEnsureIndexTemplate extends zipkin2.elasticsearch.integration.ITEnsureIndexTemplate {
+    @Override protected ElasticsearchStorage.Builder newStorageBuilder(TestInfo testInfo) {


as other tests we add to ITEnsureIndexTemplate won't necessarily effect templatePriority, I would move that setting into the test itself. Then, you can use the same setup as the other tests like this:

@Override protected ElasticsearchStorage.Builder newStorageBuilder(TestInfo testInfo) { return backend().computeStorageBuilder().index(index(testInfo)); } @AfterEach public void clear() throws IOException { storage.clear(); }

codefromthecrypt · 2020-08-30T00:22:39Z

...age/elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITEnsureIndexTemplate.java

+  /**
+   * Returns a new {@link ElasticsearchStorage.Builder} for connecting to the backend for the test.
+   */
+  protected abstract ElasticsearchStorage.Builder newStorageBuilder(TestInfo testInfo);


nit protected not needed in the same package

codefromthecrypt · 2020-08-30T00:32:15Z

...age/elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITEnsureIndexTemplate.java

+      + "      \"number_of_shards\" : 1\n"
+      + "    },\n"
+      + "    \"mappings\" : {\n"
+      + "      \"_source\" : { \"enabled\" : true }\n"


if this is the catch all change you want to make, it would be useful to test that this applies. The test could pass for a subtle reason, then in ES 7.10 it might break and we wouldn't know what it was trying to do either.

For this reason, making a template that sets default which I think this does, maybe isn't a good choice.
Instead try something with semantic value and also relevant in tracing? ex

"mappings": { "properties": { "tags.http.status_code": { "norms": false, "type": "keyword" } } }

In this case, your test can switch from making a fake tag "queryTest"->"ok" to a tag very commonly requested like "http.status_code" "404"

Then, you can verify your catch-all worked by using HTTP to do the following (except in java):

curl -s 'localhost:9200/zipkin-span-*/_search?q=tags.http.status_code:404'

Ex what you are doing is literally verifying automatically this https://gist.github.com/adriancole/1af1259102e7a2da1b3c9103565165d7

I don't think the suggested secondary template works anymore, given that ES no longer allows multiple matching index templates of the same priority. ES won't merge templates based on order anymore, and will only select 1 with the highest priority. Multiple index template with matching index patterns and priorities will result in error on PUT of the second index template. So in this case, it's either Zipkin's template or catch-all template, and I can't sneak in tags to zipkin-span-* like that.

I do see what you mean, though. And if the objective is to test the catch-all, I think I have to use a new index that doesn't match Zipkin's template at all. Thought?

interesting.. thanks for the info! yeah a comment should say this is the case now ( I remember hints of this in the past from @xeraa ). I do think the default template should meddle with ours in a way that would be obvious then. For example, you could change something we don't set? then read back that it is unset (as opposed to inherited/merged) would that do it?

meanwhile, using http in your test is still probably better as it is more realistic data, but yeah I can now expect people who setup multiple templates to have to do an offline merge of them instead of my gist now!

There are the legacy / v1 templates that can be merged, but I would be careful (multiple templates with the same order will result in a non-deterministic merging order, merging valid templates might lead to an invalid mapping). Right now they are deprecated and we'll see when we'll remove them — you have at least until 8.0.

The new component based approach still allows you to combine multiple pieces, but you don't merge them (to avoid the two problems mentioned above). This feature was only added in 7.8, so I'm not sure what will be the sanest approach for you — support it in 7.x already or switch over for any (upcoming) 8.x cluster.

ok so legacy/v1 would "for some time" retain the current behavior. I suppose in this case this means people in-place upgrading continue to work as this logic only applies when the templates don't already exist. cc also @basvanbeek for perspective on your schema tool. This chat will likely outgrow this PR :P

@ccharnkij for you and to not cage you to this decision tree, I would make a comment to the effect that @xeraa made in point 1. in your unit test and possibly also revise the javadoc/README if doesn't already say this. Basically, this relies on that we are not using legacy/v1 templates, so it is a win, not a merge.

hence we expect them to not merge. So, as mentioned earlier to show that actually happened (sanity check), have your template override something we don't and then magically notice that after the storage component initializes, whatever that setting is, is now ignored.

ccharnkij · 2020-09-01T04:25:33Z

@adriancole how about this one?

Regarding the legacy vs new template though, perhaps I could help more with it. I could allow both configuration based on environment variable, maybe ES_TEMPLATE_TYPE=legacy or new (or something like that). For ES >= 7.8, people can choose if they still want to use the legacy one or switch over to the one. Thought?

codefromthecrypt · 2020-09-01T04:48:31Z

Thanks for the volunteer to move forward. I'm a fan of least config possible.

The template priority is a new config. Maybe we can use this in such a way that if you set it, you accept using "new templates" failing that defaults to legacy ones. If this is logical, it clears up the mixed state of 7.x and at any rate 8.0 won't support legacy ones. wdyt?

ccharnkij · 2020-09-01T05:00:12Z

That works too. I just have to remove the implicit default priority=0 when not set. People that want to use new template just have to set priority all the time even if they want priority=0, until legacy template are completely removed.

codefromthecrypt · 2020-09-01T05:10:35Z

yep sgtm!

ccharnkij · 2020-09-02T08:18:26Z

@adriancole how about this one?

codefromthecrypt

Looks good. one minor thing and I think we're done

codefromthecrypt · 2020-09-02T08:57:06Z

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/ElasticsearchStorage.java

@@ -213,6 +222,8 @@ public final Builder dateSeparator(char dateSeparator) {

  public abstract int namesLookback();

+  @Nullable abstract String templatePriority();


probably using Zipkin's Nullable for Integer is better, just to prevent a little brain break

codefromthecrypt · 2020-09-02T08:58:40Z

zipkin-server/src/main/resources/zipkin-server-shared.yml

@@ -164,6 +164,7 @@ zipkin:
      health-check:
        enabled: ${ES_HEALTH_CHECK_ENABLED:true}
        interval: ${ES_HEALTH_CHECK_INTERVAL:3s}
+      priority: ${ES_TEMPLATE_PRIORITY:0}


to match your docs..

Suggested change

priority: ${ES_TEMPLATE_PRIORITY:0}

priority: ${ES_TEMPLATE_PRIORITY:}

codefromthecrypt · 2020-09-02T08:59:03Z

...rc/main/java/zipkin2/server/internal/elasticsearch/ZipkinElasticsearchStorageProperties.java

@@ -212,6 +213,8 @@ public void setInterval(Duration interval) {

  private HealthCheck healthCheck = new HealthCheck();

+  private String priority;


make Integer

codefromthecrypt · 2020-09-02T09:00:27Z

...rc/main/java/zipkin2/server/internal/elasticsearch/ZipkinElasticsearchStorageProperties.java

@@ -346,6 +349,10 @@ public void setSsl(Ssl ssl) {
    this.ssl = ssl;
  }

+  public String getPriority() { return priority; }
+
+  public void setPriority(String priority) { this.priority = priority; }


If this is a string to coerce empty to null, make a comment (also coerce empty to null :P) but still use an integer in the builder. I'm not actually sure what Spring does to an integer field set to ""

codefromthecrypt

sorry I didn't notice the naming thing earlier

codefromthecrypt · 2020-09-03T08:37:50Z

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/ElasticsearchStorage.java

+     *
+     * <p>See https://www.elastic.co/guide/en/elasticsearch/reference/7.8/_index_template_and_settings_priority.html
+     */
+    @Nullable public abstract Builder templatePriority(Integer priority);


Suggested change

@Nullable public abstract Builder templatePriority(Integer priority);

public abstract Builder templatePriority(@Nullable Integer priority);

codefromthecrypt · 2020-09-03T08:39:27Z

zipkin-server/src/main/resources/zipkin-server-shared.yml

@@ -164,6 +164,7 @@ zipkin:
      health-check:
        enabled: ${ES_HEALTH_CHECK_ENABLED:true}
        interval: ${ES_HEALTH_CHECK_INTERVAL:3s}
+      priority: ${ES_TEMPLATE_PRIORITY:}


sorry I didn't notice this earlier: rename to template-priority

add a test in ZipkinElasticsearchStorageConfigurationTest to ensure that zipkin.storage.elasticsearch.template-priority: coerces to null not zero

codefromthecrypt · 2020-09-03T08:39:45Z

...rc/main/java/zipkin2/server/internal/elasticsearch/ZipkinElasticsearchStorageProperties.java

@@ -212,6 +213,8 @@ public void setInterval(Duration interval) {

  private HealthCheck healthCheck = new HealthCheck();

+  private Integer priority;


rename to templatePriority

codefromthecrypt · 2020-09-03T08:39:52Z

...rc/main/java/zipkin2/server/internal/elasticsearch/ZipkinElasticsearchStorageProperties.java

@@ -50,6 +50,7 @@
 *     enabled: true
 *     http-logging: HEADERS
 *     interval: 3s
+ *   priority: 0


rename to template-priority

codefromthecrypt · 2020-09-04T00:21:32Z

Would you prefer ES_TEMPLATE_PRIORITY= <--- (aka null) to default to 0, instead of using legacy template? With the type set to nullable Integer, if the ES_TEMPLATE_PRIORITY is not integer (ES_TEMPLATE_PRIORITY=asfsd, for instance), Spring will throw error and fail start up. Andif ES_TEMPLATE_PRIORITY= (null), then legacy template will be used.

The main impact is the yml file which defaults this as then a user doesn't need to know how it becomes null just how to make it not null :) If spring has a way to represent a null integer, then I would recommend using that in the template-priority ${ES_TEMPLATE_PRIORITY:XXXXXX} part for XXXXXX In any case a spring properties test can ensure however we are getting to a null templatePriority works, in case spring boot changes their mind next version about how to default things.. it will be caught. make sense?

codefromthecrypt

I'm so impressed with what you've done, especially a first time committer to here. Thank you, Chanatan!

ccharnkij · 2020-09-04T01:25:07Z

You're welcome. Glad I can be of help.

codefromthecrypt · 2020-09-04T05:52:09Z

the build fail is unrelated flakey since armeria 1.0 update #3197

codefromthecrypt reviewed Aug 23, 2020

View reviewed changes

ccharnkij force-pushed the composable_template_for_es_7_8 branch from c3cf802 to 3591b6f Compare August 24, 2020 07:29

codefromthecrypt reviewed Aug 24, 2020

View reviewed changes

ccharnkij force-pushed the composable_template_for_es_7_8 branch from 3591b6f to 4a45de4 Compare August 28, 2020 06:41

codefromthecrypt reviewed Aug 28, 2020

View reviewed changes

ccharnkij force-pushed the composable_template_for_es_7_8 branch from 4a45de4 to f1e1449 Compare August 29, 2020 10:37

codefromthecrypt reviewed Aug 30, 2020

View reviewed changes

ccharnkij force-pushed the composable_template_for_es_7_8 branch from f1e1449 to 1564380 Compare September 1, 2020 04:17

ccharnkij force-pushed the composable_template_for_es_7_8 branch from 1564380 to c36cf32 Compare September 2, 2020 08:13

codefromthecrypt reviewed Sep 2, 2020

View reviewed changes

fredy-virguez-adl mentioned this pull request Sep 3, 2020

Show new lines in UI #3195

Closed

ccharnkij force-pushed the composable_template_for_es_7_8 branch from c36cf32 to a0919a8 Compare September 3, 2020 07:53

codefromthecrypt reviewed Sep 3, 2020

View reviewed changes

Composable index templates for ES 7.8

687de3a

ccharnkij force-pushed the composable_template_for_es_7_8 branch from a0919a8 to 687de3a Compare September 4, 2020 00:54

codefromthecrypt approved these changes Sep 4, 2020

View reviewed changes

codefromthecrypt merged commit 346ac51 into openzipkin:master Sep 4, 2020

codefromthecrypt mentioned this pull request Dec 10, 2020

zipkin makes tags object disabled (enable = false) when creating indices in ES openzipkin/zipkin-support#46

Open


		storage.spanConsumer().accept(spans).execute();

		assertThat(storage.traces().getTraces(traceIds).execute())

		@@ -213,6 +222,8 @@ public final Builder dateSeparator(char dateSeparator) {

		public abstract int namesLookback();

		@Nullable abstract String templatePriority();

	priority: ${ES_TEMPLATE_PRIORITY:0}
	priority: ${ES_TEMPLATE_PRIORITY:}

		@@ -212,6 +213,8 @@ public void setInterval(Duration interval) {

		private HealthCheck healthCheck = new HealthCheck();

		private String priority;

	@Nullable public abstract Builder templatePriority(Integer priority);
	public abstract Builder templatePriority(@Nullable Integer priority);

		@@ -212,6 +213,8 @@ public void setInterval(Duration interval) {

		private HealthCheck healthCheck = new HealthCheck();

		private Integer priority;

Composable index templates for ES 7.8 #3185

Composable index templates for ES 7.8 #3185

Conversation

ccharnkij commented Aug 22, 2020

Description

Type of change

How Has This Been Tested?

Checklist:

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccharnkij commented Aug 24, 2020

codefromthecrypt commented Aug 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt 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

ccharnkij commented Aug 28, 2020

codefromthecrypt 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

Choose a reason for hiding this comment

ccharnkij commented Aug 29, 2020

codefromthecrypt 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

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

ccharnkij commented Sep 1, 2020

codefromthecrypt commented Sep 1, 2020

ccharnkij commented Sep 1, 2020

codefromthecrypt commented Sep 1, 2020

ccharnkij commented Sep 2, 2020

codefromthecrypt 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

Choose a reason for hiding this comment

codefromthecrypt 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

Choose a reason for hiding this comment

codefromthecrypt commented Sep 4, 2020 via email

codefromthecrypt left a comment

Choose a reason for hiding this comment

ccharnkij commented Sep 4, 2020

codefromthecrypt commented Sep 4, 2020