Always use deprecateAndMaybeLog for deprecation warnings (elastic#55115)

Backport of elastic#55115. Closes elastic#53137. Replace calls to deprecate(String,Object...) with deprecateAndMaybeLog(...), with an appropriate key, so that all messages can potentially be deduplicated.
pugnascotia · Apr 16, 2020 · 6b26209 · 6b26209
1 parent 7941f4a
commit 6b26209
Show file tree

Hide file tree

Showing 41 changed files with 115 additions and 116 deletions.
diff --git a/...sis-common/src/main/java/org/elasticsearch/analysis/common/SynonymTokenFilterFactory.java b/...sis-common/src/main/java/org/elasticsearch/analysis/common/SynonymTokenFilterFactory.java
@@ -59,7 +59,8 @@ public class SynonymTokenFilterFactory extends AbstractTokenFilterFactory {
         this.settings = settings;
 
         if (settings.get("ignore_case") != null) {
-            DEPRECATION_LOGGER.deprecated(
+            DEPRECATION_LOGGER.deprecatedAndMaybeLog(
+                "synonym_ignore_case_option",
                 "The ignore_case option on the synonym_graph filter is deprecated. " +
                     "Instead, insert a lowercase filter in the filter chain before the synonym_graph filter.");
         }

diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/ReindexValidator.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/ReindexValidator.java
@@ -67,7 +67,7 @@ void initialValidation(ReindexRequest request) {
             state);
         SearchSourceBuilder searchSource = request.getSearchRequest().source();
         if (searchSource != null && searchSource.sorts() != null && searchSource.sorts().isEmpty() == false) {
-            deprecationLogger.deprecated(SORT_DEPRECATED_MESSAGE);
+            deprecationLogger.deprecatedAndMaybeLog("reindex_sort", SORT_DEPRECATED_MESSAGE);
         }
     }
 

diff --git a/.../src/main/java/org/elasticsearch/plugin/discovery/azure/classic/AzureDiscoveryPlugin.java b/.../src/main/java/org/elasticsearch/plugin/discovery/azure/classic/AzureDiscoveryPlugin.java
@@ -48,7 +48,7 @@ public class AzureDiscoveryPlugin extends Plugin implements DiscoveryPlugin {
 
     public AzureDiscoveryPlugin(Settings settings) {
         this.settings = settings;
-        deprecationLogger.deprecated("azure classic discovery plugin is deprecated.");
+        deprecationLogger.deprecatedAndMaybeLog("azure_discovery_plugin", "azure classic discovery plugin is deprecated.");
         logger.trace("starting azure classic discovery plugin...");
     }
 

diff --git a/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2ClientSettings.java b/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/Ec2ClientSettings.java
@@ -135,11 +135,13 @@ static AWSCredentials loadCredentials(Settings settings) {
                 return null;
             } else {
                 if (key.length() == 0) {
-                    deprecationLogger.deprecated("Setting [{}] is set but [{}] is not, which will be unsupported in future",
+                    deprecationLogger.deprecatedAndMaybeLog("ec2_invalid_settings",
+                        "Setting [{}] is set but [{}] is not, which will be unsupported in future",
                         SECRET_KEY_SETTING.getKey(), ACCESS_KEY_SETTING.getKey());
                 }
                 if (secret.length() == 0) {
-                    deprecationLogger.deprecated("Setting [{}] is set but [{}] is not, which will be unsupported in future",
+                    deprecationLogger.deprecatedAndMaybeLog("ec2_invalid_settings",
+                       "Setting [{}] is set but [{}] is not, which will be unsupported in future",
                         ACCESS_KEY_SETTING.getKey(), SECRET_KEY_SETTING.getKey());
                 }
 

diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java
@@ -104,33 +104,6 @@ public void testLocationInfoTest() throws IOException, UserException {
         assertLogLine(events.get(4), Level.TRACE, location, "This is a trace message");
     }
 
-    public void testDeprecationLogger() throws IOException, UserException {
-        setupLogging("deprecation");
-
-        final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger("deprecation"));
-
-        final int deprecatedIterations = randomIntBetween(0, 256);
-        for (int i = 0; i < deprecatedIterations; i++) {
-            deprecationLogger.deprecated("This is a deprecation message");
-            assertWarnings("This is a deprecation message");
-        }
-
-        final String deprecationPath =
-            System.getProperty("es.logs.base_path") +
-                System.getProperty("file.separator") +
-                System.getProperty("es.logs.cluster_name") +
-                "_deprecation.log";
-        final List<String> deprecationEvents = Files.readAllLines(PathUtils.get(deprecationPath));
-        assertThat(deprecationEvents.size(), equalTo(deprecatedIterations));
-        for (int i = 0; i < deprecatedIterations; i++) {
-            assertLogLine(
-                    deprecationEvents.get(i),
-                    Level.WARN,
-                    "org.elasticsearch.common.logging.DeprecationLogger\\$2\\.run",
-                    "This is a deprecation message");
-        }
-    }
-
     public void testConcurrentDeprecationLogger() throws IOException, UserException, BrokenBarrierException, InterruptedException {
         setupLogging("deprecation");
 

diff --git a/qa/smoke-test-http/src/test/java/org/elasticsearch/http/TestDeprecatedQueryBuilder.java b/qa/smoke-test-http/src/test/java/org/elasticsearch/http/TestDeprecatedQueryBuilder.java
@@ -80,7 +80,7 @@ public String getWriteableName() {
 
     @Override
     protected Query doToQuery(QueryShardContext context) throws IOException {
-        deprecationLogger.deprecated("[{}] query is deprecated, but used on [{}] index", NAME, context.index().getName());
+        deprecationLogger.deprecatedAndMaybeLog(NAME, "[{}] query is deprecated, but used on [{}] index", NAME, context.index().getName());
 
         return Queries.newMatchAllQuery();
     }

diff --git a/qa/smoke-test-http/src/test/java/org/elasticsearch/http/TestDeprecationHeaderRestAction.java b/qa/smoke-test-http/src/test/java/org/elasticsearch/http/TestDeprecationHeaderRestAction.java
@@ -106,7 +106,7 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
             final Map<String, Object> source = parser.map();
 
             if (source.containsKey("deprecated_settings")) {
-                deprecationLogger.deprecated(DEPRECATED_USAGE);
+                deprecationLogger.deprecatedAndMaybeLog("deprecated_settings", DEPRECATED_USAGE);
 
                 settings = (List<String>)source.get("deprecated_settings");
             } else {

diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
@@ -186,7 +186,8 @@ public void validateDotIndex(String index, ClusterState state, @Nullable Boolean
                 .filter(descriptor -> descriptor.matchesIndexPattern(index))
                 .collect(toList());
             if (matchingDescriptors.isEmpty() && (isHidden == null || isHidden == Boolean.FALSE)) {
-                DEPRECATION_LOGGER.deprecated("index name [{}] starts with a dot '.', in the next major version, index names " +
+                DEPRECATION_LOGGER.deprecatedAndMaybeLog("index_name_starts_with_dot",
+                    "index name [{}] starts with a dot '.', in the next major version, index names " +
                     "starting with a dot are reserved for hidden indices and system indices", index);
             } else if (matchingDescriptors.size() > 1) {
                 // This should be prevented by erroring on overlapping patterns at startup time, but is here just in case.

diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
@@ -333,7 +333,7 @@ ClusterState addIndexTemplateV2(final ClusterState currentState, final boolean c
                     .collect(Collectors.joining(",")),
                 name);
             logger.warn(warning);
-            deprecationLogger.deprecated(warning);
+            deprecationLogger.deprecatedAndMaybeLog("index_template_pattern_overlap", warning);
         }
 
         IndexTemplateV2 finalIndexTemplate = template;
@@ -558,7 +558,7 @@ static ClusterState innerPutTemplate(final ClusterState currentState, PutRequest
                         .collect(Collectors.joining(",")),
                     request.name);
                 logger.warn(warning);
-                deprecationLogger.deprecated(warning);
+                deprecationLogger.deprecatedAndMaybeLog("index_template_pattern_overlap", warning);
             } else {
                 // Otherwise, this is a hard error, the user should use V2 index templates instead
                 String error = String.format(Locale.ROOT, "template [%s] has index patterns %s matching patterns" +

diff --git a/server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java b/server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java
@@ -28,6 +28,7 @@
 import org.elasticsearch.tasks.Task;
 
 import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.util.BitSet;
@@ -110,15 +111,8 @@ public DeprecationLogger(Logger parentLogger) {
         this.logger = LogManager.getLogger(name);
     }
 
-    /**
-     * Logs a deprecation message, adding a formatted warning message as a response header on the thread context.
-     */
-    public void deprecated(String msg, Object... params) {
-        deprecated(THREAD_CONTEXT, msg, params);
-    }
-
     // LRU set of keys used to determine if a deprecation message should be emitted to the deprecation logs
-    private Set<String> keys = Collections.newSetFromMap(Collections.synchronizedMap(new LinkedHashMap<String, Boolean>() {
+    private final Set<String> keys = Collections.newSetFromMap(Collections.synchronizedMap(new LinkedHashMap<>() {
         @Override
         protected boolean removeEldestEntry(final Map.Entry<String, Boolean> eldest) {
             return size() > 128;
@@ -135,8 +129,8 @@ protected boolean removeEldestEntry(final Map.Entry<String, Boolean> eldest) {
      */
     public void deprecatedAndMaybeLog(final String key, final String msg, final Object... params) {
         String xOpaqueId = getXOpaqueId(THREAD_CONTEXT);
-        boolean log = keys.add(xOpaqueId + key);
-        deprecated(THREAD_CONTEXT, msg, log, params);
+        boolean shouldLog = keys.add(xOpaqueId + key);
+        deprecated(THREAD_CONTEXT, msg, shouldLog, params);
     }
 
     /*
@@ -234,7 +228,7 @@ void deprecated(final Set<ThreadContext> threadContexts, final String message, f
         deprecated(threadContexts, message, true, params);
     }
 
-    void deprecated(final Set<ThreadContext> threadContexts, final String message, final boolean log, final Object... params) {
+    void deprecated(final Set<ThreadContext> threadContexts, final String message, final boolean shouldLog, final Object... params) {
         final Iterator<ThreadContext> iterator = threadContexts.iterator();
         if (iterator.hasNext()) {
             final String formattedMessage = LoggerMessageFormat.format(message, params);
@@ -251,12 +245,12 @@ void deprecated(final Set<ThreadContext> threadContexts, final String message, f
             }
         }
 
-        if (log) {
+        if (shouldLog) {
             AccessController.doPrivileged(new PrivilegedAction<Void>() {
                 @SuppressLoggerChecks(reason = "safely delegates to logger")
                 @Override
                 public Void run() {
-                    /**
+                    /*
                      * There should be only one threadContext (in prod env), @see DeprecationLogger#setThreadContext
                      */
                     String opaqueId = getXOpaqueId(threadContexts);
@@ -360,7 +354,7 @@ static String escapeBackslashesAndQuotes(final String s) {
         assert doesNotNeedEncoding.get('%') == false : doesNotNeedEncoding;
     }
 
-    private static final Charset UTF_8 = Charset.forName("UTF-8");
+    private static final Charset UTF_8 = StandardCharsets.UTF_8;
 
     /**
      * Encode a string containing characters outside of the legal characters for an RFC 7230 quoted-string.

diff --git a/server/src/main/java/org/elasticsearch/common/unit/ByteSizeValue.java b/server/src/main/java/org/elasticsearch/common/unit/ByteSizeValue.java
@@ -245,7 +245,8 @@ private static ByteSizeValue parse(final String initialInput, final String norma
             } catch (final NumberFormatException e) {
                 try {
                     final double doubleValue = Double.parseDouble(s);
-                    DeprecationLoggerHolder.deprecationLogger.deprecated(
+                    DeprecationLoggerHolder.deprecationLogger.deprecatedAndMaybeLog(
+                            "fractional_byte_values",
                             "Fractional bytes values are deprecated. Use non-fractional bytes values instead: [{}] found for setting [{}]",
                             initialInput, settingName);
                     return new ByteSizeValue((long) (doubleValue * unit.toBytes(1)));

diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/LoggingDeprecationHandler.java b/server/src/main/java/org/elasticsearch/common/xcontent/LoggingDeprecationHandler.java
@@ -53,21 +53,21 @@ private LoggingDeprecationHandler() {
     @Override
     public void usedDeprecatedName(String parserName, Supplier<XContentLocation> location, String usedName, String modernName) {
         String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
-        deprecationLogger.deprecated("{}Deprecated field [{}] used, expected [{}] instead",
+        deprecationLogger.deprecatedAndMaybeLog("deprecated_field", "{}Deprecated field [{}] used, expected [{}] instead",
             prefix, usedName, modernName);
     }
 
     @Override
     public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName, String replacedWith) {
         String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
-        deprecationLogger.deprecated("{}Deprecated field [{}] used, replaced by [{}]",
+        deprecationLogger.deprecatedAndMaybeLog("deprecated_field", "{}Deprecated field [{}] used, replaced by [{}]",
             prefix, usedName, replacedWith);
     }
 
     @Override
     public void usedDeprecatedField(String parserName, Supplier<XContentLocation> location, String usedName) {
         String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
-        deprecationLogger.deprecated("{}Deprecated field [{}] used, this field is unused and will be removed entirely",
-            prefix, usedName);
+        deprecationLogger.deprecatedAndMaybeLog("deprecated_field",
+            "{}Deprecated field [{}] used, this field is unused and will be removed entirely", prefix, usedName);
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/analysis/ShingleTokenFilterFactory.java b/server/src/main/java/org/elasticsearch/index/analysis/ShingleTokenFilterFactory.java
@@ -51,7 +51,8 @@ public ShingleTokenFilterFactory(IndexSettings indexSettings, Environment enviro
                         + " must be less than or equal to: [" + maxAllowedShingleDiff + "] but was [" + shingleDiff + "]. This limit"
                         + " can be set by changing the [" + IndexSettings.MAX_SHINGLE_DIFF_SETTING.getKey() + "] index level setting.");
             } else {
-                deprecationLogger.deprecated("Deprecated big difference between maxShingleSize and minShingleSize" +
+                deprecationLogger.deprecatedAndMaybeLog("excessive_shingle_diff",
+                    "Deprecated big difference between maxShingleSize and minShingleSize" +
                     " in Shingle TokenFilter, expected difference must be less than or equal to: [" + maxAllowedShingleDiff + "]");
             }
         }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java
@@ -419,7 +419,8 @@ public CompletionFieldMapper build(BuilderContext context) {
 
         private void checkCompletionContextsLimit(BuilderContext context) {
             if (this.contextMappings != null && this.contextMappings.size() > COMPLETION_CONTEXTS_LIMIT) {
-                deprecationLogger.deprecated("You have defined more than [" + COMPLETION_CONTEXTS_LIMIT + "] completion contexts" +
+                deprecationLogger.deprecatedAndMaybeLog("excessive_completion_contexts",
+                    "You have defined more than [" + COMPLETION_CONTEXTS_LIMIT + "] completion contexts" +
                     " in the mapping for index [" + context.indexSettings().get(IndexMetadata.SETTING_INDEX_PROVIDED_NAME) + "]. " +
                     "The maximum allowed number of completion contexts in a mapping will be limited to " +
                     "[" + COMPLETION_CONTEXTS_LIMIT + "] starting in version [8.0].");

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java
@@ -185,7 +185,8 @@ public Query termQuery(Object value, QueryShardContext context) {
             if (isEnabled() == false) {
                 throw new IllegalStateException("Cannot run [exists] queries if the [_field_names] field is disabled");
             }
-            deprecationLogger.deprecated(
+            deprecationLogger.deprecatedAndMaybeLog(
+                    "terms_query_on_field_names",
                     "terms query on the _field_names field is deprecated and will be removed, use exists query instead");
             return super.termQuery(value, context);
         }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java
@@ -170,8 +170,8 @@ private static void checkPrefixTreeSupport(String fieldName) {
                 throw new ElasticsearchParseException("Field parameter [{}] is not supported for [{}] field type",
                     fieldName, CONTENT_TYPE);
             }
-            DEPRECATION_LOGGER.deprecated("Field parameter [{}] is deprecated and will be removed in a future version.",
-                fieldName);
+            DEPRECATION_LOGGER.deprecatedAndMaybeLog("geo_mapper_field_parameter",
+                "Field parameter [{}] is deprecated and will be removed in a future version.", fieldName);
         }
     }
 

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
@@ -197,7 +197,8 @@ protected static boolean parseObjectOrDocumentTypeProperties(String fieldName, O
                 }
                 return true;
             } else if (fieldName.equals("include_in_all")) {
-                deprecationLogger.deprecated("[include_in_all] is deprecated, the _all field have been removed in this version");
+                deprecationLogger.deprecatedAndMaybeLog("include_in_all",
+                    "[include_in_all] is deprecated, the _all field have been removed in this version");
                 return true;
             }
             return false;

diff --git a/...src/main/java/org/elasticsearch/index/query/functionscore/RandomScoreFunctionBuilder.java b/...src/main/java/org/elasticsearch/index/query/functionscore/RandomScoreFunctionBuilder.java
@@ -168,7 +168,7 @@ protected ScoreFunction doToFunction(QueryShardContext context) {
             if (field != null) {
                 fieldType = context.getMapperService().fieldType(field);
             } else {
-                deprecationLogger.deprecated(
+                deprecationLogger.deprecatedAndMaybeLog("seed_requires_field",
                         "As of version 7.0 Elasticsearch will require that a [field] parameter is provided when a [seed] is set");
                 fieldType = context.getMapperService().fieldType(IdFieldMapper.NAME);
             }

diff --git a/server/src/main/java/org/elasticsearch/index/similarity/SimilarityProviders.java b/server/src/main/java/org/elasticsearch/index/similarity/SimilarityProviders.java
@@ -143,7 +143,7 @@ private static BasicModel parseBasicModel(Version indexCreatedVersion, Settings
                     throw new IllegalArgumentException("Basic model [" + basicModel + "] isn't supported anymore, " +
                         "please use another model.");
                 } else {
-                    deprecationLogger.deprecated("Basic model [" + basicModel +
+                    deprecationLogger.deprecatedAndMaybeLog(basicModel + "_similarity_model_replaced", "Basic model [" + basicModel +
                         "] isn't supported anymore and has arbitrarily been replaced with [" + replacement + "].");
                     model = BASIC_MODELS.get(replacement);
                     assert model != null;
@@ -174,7 +174,7 @@ private static AfterEffect parseAfterEffect(Version indexCreatedVersion, Setting
                     throw new IllegalArgumentException("After effect [" + afterEffect +
                         "] isn't supported anymore, please use another effect.");
                 } else {
-                    deprecationLogger.deprecated("After effect [" + afterEffect +
+                    deprecationLogger.deprecatedAndMaybeLog(afterEffect + "_after_effect_replaced", "After effect [" + afterEffect +
                         "] isn't supported anymore and has arbitrarily been replaced with [" + replacement + "].");
                     effect = AFTER_EFFECTS.get(replacement);
                     assert effect != null;
@@ -264,7 +264,8 @@ static void assertSettingsIsSubsetOf(String type, Version version, Settings sett
             if (version.onOrAfter(Version.V_7_0_0)) {
                 throw new IllegalArgumentException("Unknown settings for similarity of type [" + type + "]: " + unknownSettings);
             } else {
-                deprecationLogger.deprecated("Unknown settings for similarity of type [" + type + "]: " + unknownSettings);
+                deprecationLogger.deprecatedAndMaybeLog("unknown_similarity_setting",
+                    "Unknown settings for similarity of type [" + type + "]: " + unknownSettings);
             }
         }
     }

diff --git a/server/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java b/server/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java
@@ -154,7 +154,8 @@ public SimilarityService(IndexSettings indexSettings, ScriptService scriptServic
         defaultSimilarity = (providers.get("default") != null) ? providers.get("default").get()
                                                               : providers.get(SimilarityService.DEFAULT_SIMILARITY).get();
         if (providers.get("base") != null) {
-            deprecationLogger.deprecated("The [base] similarity is ignored since query normalization and coords have been removed");
+            deprecationLogger.deprecatedAndMaybeLog("base_similarity_ignored",
+                "The [base] similarity is ignored since query normalization and coords have been removed");
         }
     }
 

diff --git a/server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java b/server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java
@@ -57,7 +57,7 @@ public DeprecationRestHandler(RestHandler handler, String deprecationMessage, De
      */
     @Override
     public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
-        deprecationLogger.deprecated(deprecationMessage);
+        deprecationLogger.deprecatedAndMaybeLog("deprecated_route", deprecationMessage);
 
         handler.handleRequest(request, channel, client);
     }