zipkin-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From adrianc...@apache.org
Subject [incubator-zipkin] branch master updated: Simplifies Elasticsearch index template when strict trace ID is enabled (#2561)
Date Tue, 07 May 2019 03:04:58 GMT
This is an automated email from the ASF dual-hosted git repository.

adriancole pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-zipkin.git


The following commit(s) were added to refs/heads/master by this push:
     new 2a55aa1  Simplifies Elasticsearch index template when strict trace ID is enabled
(#2561)
2a55aa1 is described below

commit 2a55aa1c7a1d2fd82b148ed8ad166ff89e7a8b80
Author: Adrian Cole <adriancole@users.noreply.github.com>
AuthorDate: Tue May 7 11:04:49 2019 +0800

    Simplifies Elasticsearch index template when strict trace ID is enabled (#2561)
    
    By default, we use strict trace IDs. This removes the analysis section
    of the index template when that's the case, simplifying it.
---
 .../java/zipkin2/elasticsearch/IndexTemplates.java |  13 +--
 .../elasticsearch/VersionSpecificTemplates.java    | 108 +++++++++++++--------
 .../java/zipkin2/elasticsearch/TestResponses.java  |   8 +-
 .../VersionSpecificTemplatesTest.java              |  22 ++++-
 .../java/zipkin2/storage/StorageComponent.java     |   4 +-
 5 files changed, 97 insertions(+), 58 deletions(-)

diff --git a/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/IndexTemplates.java
b/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/IndexTemplates.java
index 41e6f8c..74c23f7 100644
--- a/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/IndexTemplates.java
+++ b/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/IndexTemplates.java
@@ -26,25 +26,20 @@ abstract class IndexTemplates {
 
   abstract float version();
 
+  abstract char indexTypeDelimiter();
+
   abstract String span();
 
   abstract String dependency();
 
   abstract String autocomplete();
 
-  /**
-   * This returns a delimiter based on what's supported by the Elasticsearch version.
-   *
-   * <p>See https://github.com/openzipkin/zipkin/issues/2219
-   */
-  char indexTypeDelimiter() {
-    return version() < 7 ? ':' : '-';
-  }
-
   @AutoValue.Builder
   interface Builder {
     Builder version(float version);
 
+    Builder indexTypeDelimiter(char indexTypeDelimiter);
+
     Builder span(String span);
 
     Builder dependency(String dependency);
diff --git a/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/VersionSpecificTemplates.java
b/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/VersionSpecificTemplates.java
index 1db7eeb..244aa87 100644
--- a/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/VersionSpecificTemplates.java
+++ b/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/VersionSpecificTemplates.java
@@ -22,48 +22,56 @@ import okhttp3.Request;
 import okio.BufferedSource;
 import zipkin2.elasticsearch.internal.client.HttpCall;
 
-import static zipkin2.elasticsearch.ElasticsearchAutocompleteTags.AUTOCOMPLETE;
-import static zipkin2.elasticsearch.ElasticsearchSpanStore.DEPENDENCY;
-import static zipkin2.elasticsearch.ElasticsearchSpanStore.SPAN;
 import static zipkin2.elasticsearch.internal.JsonReaders.enterPath;
 
 /** Returns a version-specific span and dependency index template */
 final class VersionSpecificTemplates {
+  /**
+   * In Zipkin search, we do exact match only (keyword). Norms is about scoring. We don't
use that
+   * in our API, and disable it to reduce disk storage needed.
+   */
   static final String KEYWORD = "{ \"type\": \"keyword\", \"norms\": false }";
 
-  final boolean searchEnabled;
+  final boolean searchEnabled, strictTraceId;
   final String spanIndexTemplate;
   final String dependencyIndexTemplate;
   final String autocompleteIndexTemplate;
 
   VersionSpecificTemplates(ElasticsearchStorage es) {
     this.searchEnabled = es.searchEnabled();
+    this.strictTraceId = es.strictTraceId();
     this.spanIndexTemplate = spanIndexTemplate()
-      .replace("${__INDEX__}", es.indexNameFormatter().index())
-      .replace("${__NUMBER_OF_SHARDS__}", String.valueOf(es.indexShards()))
-      .replace("${__NUMBER_OF_REPLICAS__}", String.valueOf(es.indexReplicas()))
-      .replace("${__TRACE_ID_MAPPING__}", es.strictTraceId() ? KEYWORD
+      .replace("${INDEX}", es.indexNameFormatter().index())
+      .replace("${NUMBER_OF_SHARDS}", String.valueOf(es.indexShards()))
+      .replace("${NUMBER_OF_REPLICAS}", String.valueOf(es.indexReplicas()))
+      .replace("${TRACE_ID_MAPPING}", strictTraceId ? KEYWORD
+        // Supporting mixed trace ID length is expensive due to needing a special analyzer
and
+        // "fielddata" which consumes a lot of heap. Sites should only turn off strict trace
ID when
+        // in a transition, and keep trace ID length transitions as short time as possible.
         : "{ \"type\": \"text\", \"fielddata\": \"true\", \"analyzer\": \"traceId_analyzer\"
}");
     this.dependencyIndexTemplate = DEPENDENCY_INDEX_TEMPLATE
-      .replace("${__INDEX__}", es.indexNameFormatter().index())
-      .replace("${__NUMBER_OF_SHARDS__}", String.valueOf(es.indexShards()))
-      .replace("${__NUMBER_OF_REPLICAS__}", String.valueOf(es.indexReplicas()));
+      .replace("${INDEX}", es.indexNameFormatter().index())
+      .replace("${NUMBER_OF_SHARDS}", String.valueOf(es.indexShards()))
+      .replace("${NUMBER_OF_REPLICAS}", String.valueOf(es.indexReplicas()));
     this.autocompleteIndexTemplate = AUTOCOMPLETE_INDEX_TEMPLATE
-      .replace("${__INDEX__}", es.indexNameFormatter().index())
-      .replace("${__NUMBER_OF_SHARDS__}", String.valueOf(es.indexShards()))
-      .replace("${__NUMBER_OF_REPLICAS__}", String.valueOf(es.indexReplicas()));
+      .replace("${INDEX}", es.indexNameFormatter().index())
+      .replace("${NUMBER_OF_SHARDS}", String.valueOf(es.indexShards()))
+      .replace("${NUMBER_OF_REPLICAS}", String.valueOf(es.indexReplicas()));
   }
 
   /** Templatized due to version differences. Only fields used in search are declared */
   String spanIndexTemplate() {
-    String result =
-      "{\n"
-        + "  \"index_patterns\": \"${__INDEX__}:span-*\",\n"
-        + "  \"settings\": {\n"
-        + "    \"index.number_of_shards\": ${__NUMBER_OF_SHARDS__},\n"
-        + "    \"index.number_of_replicas\": ${__NUMBER_OF_REPLICAS__},\n"
-        + "    \"index.requests.cache.enable\": true,\n"
-        + "    \"index.mapper.dynamic\": false,\n"
+    String result = ""
+      + "{\n"
+      + "  \"index_patterns\": \"${INDEX}${INDEX_TYPE_DELIMITER}span-*\",\n"
+      + "  \"settings\": {\n"
+      + "    \"index.number_of_shards\": ${NUMBER_OF_SHARDS},\n"
+      + "    \"index.number_of_replicas\": ${NUMBER_OF_REPLICAS},\n"
+      + "    \"index.requests.cache.enable\": true,\n"
+      + "    \"index.mapper.dynamic\": false";
+
+    if (!strictTraceId) {
+      result += (",\n"
         + "    \"analysis\": {\n"
         + "      \"analyzer\": {\n"
         + "        \"traceId_analyzer\": {\n"
@@ -79,8 +87,11 @@ final class VersionSpecificTemplates {
         + "          \"preserve_original\": true\n"
         + "        }\n"
         + "      }\n"
-        + "    }\n"
-        + "  },\n";
+        + "    }\n");
+    }
+
+    result += "  },\n";
+
     if (searchEnabled) {
       return result
         + ("  \"mappings\": {\n"
@@ -90,7 +101,7 @@ final class VersionSpecificTemplates {
         + "        {\n"
         + "          \"strings\": {\n"
         + "            \"mapping\": {\n"
-        + "              \"type\": \"keyword\",\"norms\": false\n,\n"
+        + "              \"type\": \"keyword\",\"norms\": false,\n"
         + "              \"ignore_above\": 256\n"
         + "            },\n"
         + "            \"match_mapping_type\": \"string\",\n"
@@ -99,7 +110,7 @@ final class VersionSpecificTemplates {
         + "        }\n"
         + "      ],\n"
         + "      \"properties\": {\n"
-        + "        \"traceId\": ${__TRACE_ID_MAPPING__},\n"
+        + "        \"traceId\": ${TRACE_ID_MAPPING},\n"
         + "        \"name\": " + KEYWORD + ",\n"
         + "        \"localEndpoint\": {\n"
         + "          \"type\": \"object\",\n"
@@ -128,7 +139,7 @@ final class VersionSpecificTemplates {
       + ("  \"mappings\": {\n"
       + "    \"span\": {\n"
       + "      \"properties\": {\n"
-      + "        \"traceId\": ${__TRACE_ID_MAPPING__},\n"
+      + "        \"traceId\": ${TRACE_ID_MAPPING},\n"
       + "        \"annotations\": { \"enabled\": false },\n"
       + "        \"tags\": { \"enabled\": false }\n"
       + "      }\n"
@@ -140,10 +151,10 @@ final class VersionSpecificTemplates {
   /** Templatized due to version differences. Only fields used in search are declared */
   static final String DEPENDENCY_INDEX_TEMPLATE =
     "{\n"
-      + "  \"index_patterns\": \"${__INDEX__}:dependency-*\",\n"
+      + "  \"index_patterns\": \"${INDEX}${INDEX_TYPE_DELIMITER}dependency-*\",\n"
       + "  \"settings\": {\n"
-      + "    \"index.number_of_shards\": ${__NUMBER_OF_SHARDS__},\n"
-      + "    \"index.number_of_replicas\": ${__NUMBER_OF_REPLICAS__},\n"
+      + "    \"index.number_of_shards\": ${NUMBER_OF_SHARDS},\n"
+      + "    \"index.number_of_replicas\": ${NUMBER_OF_REPLICAS},\n"
       + "    \"index.requests.cache.enable\": true,\n"
       + "    \"index.mapper.dynamic\": false\n"
       + "  },\n"
@@ -154,10 +165,10 @@ final class VersionSpecificTemplates {
   // BodyConverters KEY
   static final String AUTOCOMPLETE_INDEX_TEMPLATE =
     "{\n"
-      + "  \"index_patterns\": \"${__INDEX__}:autocomplete-*\",\n"
+      + "  \"index_patterns\": \"${INDEX}${INDEX_TYPE_DELIMITER}autocomplete-*\",\n"
       + "  \"settings\": {\n"
-      + "    \"index.number_of_shards\": ${__NUMBER_OF_SHARDS__},\n"
-      + "    \"index.number_of_replicas\": ${__NUMBER_OF_REPLICAS__},\n"
+      + "    \"index.number_of_shards\": ${NUMBER_OF_SHARDS},\n"
+      + "    \"index.number_of_replicas\": ${NUMBER_OF_REPLICAS},\n"
       + "    \"index.requests.cache.enable\": true,\n"
       + "    \"index.mapper.dynamic\": false\n"
       + "  },\n"
@@ -178,12 +189,25 @@ final class VersionSpecificTemplates {
     }
     return IndexTemplates.newBuilder()
       .version(version)
-      .span(versionSpecificIndexTemplate(SPAN, spanIndexTemplate, version))
-      .dependency(versionSpecificIndexTemplate(DEPENDENCY, dependencyIndexTemplate, version))
-      .autocomplete(versionSpecificIndexTemplate(AUTOCOMPLETE, autocompleteIndexTemplate,
version))
+      .indexTypeDelimiter(indexTypeDelimiter(version))
+      .span(versionSpecificIndexTemplate(spanIndexTemplate, version))
+      .dependency(versionSpecificIndexTemplate(dependencyIndexTemplate, version))
+      .autocomplete(versionSpecificIndexTemplate(autocompleteIndexTemplate, version))
       .build();
   }
 
+  /**
+   * This returns a delimiter based on what's supported by the Elasticsearch version.
+   *
+   * <p>Starting in Elasticsearch 7.x, colons are no longer allowed in index names.
This logic will
+   * make sure the pattern in our index template doesn't use them either.
+   *
+   * <p>See https://github.com/openzipkin/zipkin/issues/2219
+   */
+  static char indexTypeDelimiter(float version) {
+    return version < 7.0f ? ':' : '-';
+  }
+
   static float getVersion(HttpCall.Factory callFactory) throws IOException {
     Request getNode = new Request.Builder().url(callFactory.baseUrl).tag("get-node").build();
     return callFactory.newCall(getNode, ReadVersionNumber.INSTANCE).execute();
@@ -198,17 +222,17 @@ final class VersionSpecificTemplates {
       String versionString = version.nextString();
       return Float.valueOf(versionString.substring(0, 3));
     }
-}
+  }
 
-  static String versionSpecificIndexTemplate(String type, String template, float version)
{
+  static String versionSpecificIndexTemplate(String template, float version) {
+    String indexTypeDelimiter = Character.toString(indexTypeDelimiter(version));
+    template = template.replace("${INDEX_TYPE_DELIMITER}", indexTypeDelimiter);
     if (version < 6.0f) return template.replace("index_patterns", "template");
     // 6.x _all disabled https://www.elastic.co/guide/en/elasticsearch/reference/6.7/breaking-changes-6.0.html#_the_literal__all_literal_meta_field_is_now_disabled_by_default
     // 7.x _default disallowed https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-7.0.html#_the_literal__default__literal_mapping_is_no_longer_allowed
     if (version < 7.0f) return template;
-    // Colons are no longer allowed in index names. Make sure the pattern in our index template
-    // doesn't use them either.
-    template = template.replaceAll(":" + type, "-" + type);
-    return template.replaceAll(",\n +\"index\\.mapper\\.dynamic\": false", "");
+    // There is no explicit documentation of this setting being removed in 7.0f, but it was.
+    return template.replace(",\n    \"index.mapper.dynamic\": false", "");
   }
 }
 
diff --git a/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/TestResponses.java
b/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/TestResponses.java
index 862a10a..e337027 100644
--- a/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/TestResponses.java
+++ b/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/TestResponses.java
@@ -16,8 +16,8 @@
  */
 package zipkin2.elasticsearch;
 
-public final class TestResponses {
-  public static final String SERVICE_NAMES =
+final class TestResponses {
+  static final String SERVICE_NAMES =
       "{\n"
           + "  \"took\": 4,\n"
           + "  \"timed_out\": false,\n"
@@ -61,7 +61,7 @@ public final class TestResponses {
           + "  }\n"
           + "}";
 
-  public static final String SPAN_NAMES =
+  static final String SPAN_NAMES =
       "{\n"
           + "  \"took\": 1,\n"
           + "  \"timed_out\": false,\n"
@@ -93,7 +93,7 @@ public final class TestResponses {
           + "  }\n"
           + "}";
 
-      public static final String AUTOCOMPLETE_VALUES = "{\n"
+      static final String AUTOCOMPLETE_VALUES = "{\n"
         + "  \"took\": 12,\n"
         + "  \"timed_out\": false,\n"
         + "  \"_shards\": {\n"
diff --git a/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/VersionSpecificTemplatesTest.java
b/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/VersionSpecificTemplatesTest.java
index 2bbc3c1..0636403 100644
--- a/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/VersionSpecificTemplatesTest.java
+++ b/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/VersionSpecificTemplatesTest.java
@@ -149,7 +149,7 @@ public class VersionSpecificTemplatesTest {
       .withFailMessage("Starting at v7.x, we delimit index and type with hyphen")
       .contains("\"index_patterns\": \"zipkin-autocomplete-*\"");
     assertThat(template.autocomplete())
-      .withFailMessage("7.x does not support the key index.mapper.dynamic")
+      //.withFailMessage("7.x does not support the key index.mapper.dynamic")
       .doesNotContain("\"index.mapper.dynamic\": false");
   }
 
@@ -174,4 +174,24 @@ public class VersionSpecificTemplatesTest {
         + "    }\n"
         + "  }");
   }
+
+  @Test public void strictTraceId_doesNotIncludeAnalysisSection() throws Exception {
+    es.enqueue(VERSION_RESPONSE_6);
+
+    IndexTemplates template = new VersionSpecificTemplates(storage).get(storage.http());
+
+    assertThat(template.span()).doesNotContain("analysis");
+  }
+
+  @Test public void strictTraceId_false_includesAnalysisForMixedLengthTraceId() throws Exception
{
+    storage = ElasticsearchStorage.newBuilder().hosts(storage.hostsSupplier().get())
+      .strictTraceId(false)
+      .build();
+
+    es.enqueue(VERSION_RESPONSE_6);
+
+    IndexTemplates template = new VersionSpecificTemplates(storage).get(storage.http());
+
+    assertThat(template.span()).contains("analysis");
+  }
 }
diff --git a/zipkin/src/main/java/zipkin2/storage/StorageComponent.java b/zipkin/src/main/java/zipkin2/storage/StorageComponent.java
index 1d7c9bb..3a6c115 100644
--- a/zipkin/src/main/java/zipkin2/storage/StorageComponent.java
+++ b/zipkin/src/main/java/zipkin2/storage/StorageComponent.java
@@ -111,8 +111,8 @@ public abstract class StorageComponent extends Component {
      * there is overhead associated with indexing spans both by 64 and 128-bit trace IDs.
When a
      * site has finished upgrading to 128-bit trace IDs, they should enable this setting.
      *
-     * <p>See https://github.com/openzipkin/b3-propagation/issues/6 for the status
of known open
-     * source libraries on 128-bit trace identifiers.
+     * <p>See https://github.com/apache/incubator-zipkin-b3-propagation/issues/6 for
the status of
+     * known open source libraries on 128-bit trace identifiers.
      */
     public abstract Builder strictTraceId(boolean strictTraceId);
 


Mime
View raw message