Fix log messages and init progress for the profile API (#374)

* Fix log messages and init progress for the profile API This PR fixes incorrect or unnecessary log messages. The PR also specifies init progress to use ASCII explicitly. Otherwise, it is possible we may get unexpected characters. Testing done: 1. gradle build
opendistro-for-elasticsearch · Jan 29, 2021 · b40e12a · b40e12a
1 parent 16631c8
commit b40e12a
Show file tree

Hide file tree

Showing 20 changed files with 128 additions and 66 deletions.
diff --git a/build.gradle b/build.gradle
@@ -108,9 +108,18 @@ configurations {
     }
 }
 
-// Allow @Test to be used in test classes not inherited from LuceneTestCase.
-// see https://github.com/elastic/elasticsearch/blob/master/buildSrc/src/main/resources/forbidden/es-test-signatures.txt
-forbiddenApis.ignoreFailures = true
+tasks.named('forbiddenApisMain').configure {
+  // Only enable limited check because AD code has too many violations.
+  replaceSignatureFiles 'jdk-signatures'
+  signaturesFiles += files('src/forbidden/ad-signatures.txt')
+}
+
+tasks.named('forbiddenApisTest').configure {
+  // Disable check because AD code has too many violations.
+  // For example, we have to allow @Test to be used in test classes not inherited from LuceneTestCase.
+  // see https://github.com/elastic/elasticsearch/blob/master/buildSrc/src/main/resources/forbidden/es-test-signatures.txt
+  ignoreFailures = true
+}
 
 // Allow test cases to be named Tests without having to be inherited from LuceneTestCase.
 // see https://github.com/elastic/elasticsearch/blob/323f312bbc829a63056a79ebe45adced5099f6e6/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java

diff --git a/src/forbidden/ad-signatures.txt b/src/forbidden/ad-signatures.txt
@@ -0,0 +1,17 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+ @defaultMessage use format with Locale
+ java.lang.String#format(java.lang.String,java.lang.Object[])
diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/AbstractProfileRunner.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/AbstractProfileRunner.java
@@ -15,6 +15,8 @@
 
 package com.amazon.opendistroforelasticsearch.ad;
 
+import java.util.Locale;
+
 import com.amazon.opendistroforelasticsearch.ad.model.InitProgressProfile;
 
 public abstract class AbstractProfileRunner {
@@ -29,7 +31,9 @@ protected InitProgressProfile computeInitProgressProfile(long totalUpdates, long
         int neededPoints = (int) (requiredSamples - totalUpdates);
         return new InitProgressProfile(
             // rounding: 93.456 => 93%, 93.556 => 94%
-            String.format("%.0f%%", percent),
+            // Without Locale.ROOT, sometimes conversions use localized decimal digits
+            // rather than the usual ASCII digits. See https://tinyurl.com/y5sdr5tp
+            String.format(Locale.ROOT, "%.0f%%", percent),
             intervalMins * neededPoints,
             neededPoints
         );

diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/AnomalyDetectorRunner.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/AnomalyDetectorRunner.java
@@ -21,6 +21,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.stream.Collectors;
 
@@ -87,7 +88,7 @@ public void executeDetector(AnomalyDetector detector, Instant startTime, Instant
                     new MultiResponsesDelegateActionListener<EntityAnomalyResult>(
                         entityAnomalyResultListener,
                         entities.size(),
-                        String.format("Fail to get preview result for multi entity detector %s", detector.getDetectorId()),
+                        String.format(Locale.ROOT, "Fail to get preview result for multi entity detector %s", detector.getDetectorId()),
                         true
                     );
                 for (Entity entity : entities) {

diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/EntityProfileRunner.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/EntityProfileRunner.java
@@ -227,8 +227,13 @@ private void getJob(
 
                             delegateListener.onResponse(builder.build());
                         }, exception -> {
-                            logger.warn("fail to get last sample time", exception);
                             // sth wrong like result index not created. Return what we have
+                            if (exception instanceof IndexNotFoundException) {
+                                // don't print out stack trace since it is not helpful
+                                logger.info("Result index hasn't been created", exception.getMessage());
+                            } else {
+                                logger.warn("fail to get last sample time", exception);
+                            }
                             delegateListener.onResponse(builder.build());
                         }));
                     }

diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/MemoryTracker.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/MemoryTracker.java
@@ -18,6 +18,7 @@
 import static com.amazon.opendistroforelasticsearch.ad.settings.AnomalyDetectorSettings.MODEL_MAX_SIZE_PERCENTAGE;
 
 import java.util.EnumMap;
+import java.util.Locale;
 import java.util.Map;
 
 import org.apache.logging.log4j.LogManager;
@@ -102,6 +103,7 @@ public synchronized boolean canAllocateReserved(String detectorId, long required
                 detectorId,
                 String
                     .format(
+                        Locale.ROOT,
                         "Exceeded memory limit. New size is %d bytes and max limit is %d bytes",
                         reservedMemoryBytes + requiredBytes,
                         heapLimitBytes
@@ -247,6 +249,7 @@ public synchronized boolean syncMemoryState(Origin origin, long totalBytes, long
             .info(
                 String
                     .format(
+                        Locale.ROOT,
                         "Memory states do not match.  Recorded: total bytes %d, reserved bytes %d."
                             + "Actual: total bytes %d, reserved bytes: %d",
                         recordedTotalBytes,

diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/feature/SearchFeatureDao.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/feature/SearchFeatureDao.java
@@ -31,6 +31,7 @@
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
@@ -418,7 +419,7 @@ public Optional<Entry<double[][], Integer>> getFeaturesForSampledPeriods(
         Map<Long, double[]> cache = new HashMap<>();
         int currentStride = maxStride;
         Optional<double[][]> features = Optional.empty();
-        logger.info(String.format("Getting features for detector %s starting %d", detector.getDetectorId(), endTime));
+        logger.info(String.format(Locale.ROOT, "Getting features for detector %s starting %d", detector.getDetectorId(), endTime));
         while (currentStride >= 1) {
             boolean isInterpolatable = currentStride < maxStride;
             features = getFeaturesForSampledPeriods(detector, maxSamples, currentStride, endTime, cache, isInterpolatable);
@@ -428,6 +429,7 @@ public Optional<Entry<double[][], Integer>> getFeaturesForSampledPeriods(
                     .info(
                         String
                             .format(
+                                Locale.ROOT,
                                 "Get features for detector %s finishes with features present %b, current stride %d",
                                 detector.getDetectorId(),
                                 features.isPresent(),
@@ -511,7 +513,7 @@ public void getFeaturesForSampledPeriods(
         ActionListener<Optional<Entry<double[][], Integer>>> listener
     ) {
         Map<Long, double[]> cache = new HashMap<>();
-        logger.info(String.format("Getting features for detector %s ending at %d", detector.getDetectorId(), endTime));
+        logger.info(String.format(Locale.ROOT, "Getting features for detector %s ending at %d", detector.getDetectorId(), endTime));
         getFeatureSamplesWithCache(detector, maxSamples, maxStride, endTime, cache, maxStride, listener);
     }
 
@@ -563,6 +565,7 @@ private void processFeatureSamplesForStride(
                 .info(
                     String
                         .format(
+                            Locale.ROOT,
                             "Get features for detector %s finishes without any features present, current stride %d",
                             detector.getDetectorId(),
                             currentStride
@@ -574,6 +577,7 @@ private void processFeatureSamplesForStride(
                 .info(
                     String
                         .format(
+                            Locale.ROOT,
                             "Get features for detector %s finishes with %d samples, current stride %d",
                             detector.getDetectorId(),
                             features.get().length,

diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/CheckpointDao.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/CheckpointDao.java
@@ -27,6 +27,7 @@
 import java.util.Arrays;
 import java.util.ConcurrentModificationException;
 import java.util.HashMap;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
@@ -223,7 +224,7 @@ private void onCheckpointNotExist(Map<String, Object> source, String modelId, bo
                     saveModelCheckpointSync(source, modelId);
                 }
             } else {
-                logger.error(String.format("Unexpected error creating index %s", indexName), exception);
+                logger.error(String.format(Locale.ROOT, "Unexpected error creating index %s", indexName), exception);
             }
         }));
     }
@@ -271,7 +272,7 @@ public void flush() {
                             // It is possible the index has been created while we sending the create request
                             flush(bulkRequest);
                         } else {
-                            logger.error(String.format("Unexpected error creating index %s", indexName), exception);
+                            logger.error(String.format(Locale.ROOT, "Unexpected error creating index %s", indexName), exception);
                         }
                     }));
                 }

diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/ModelManager.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/ModelManager.java
@@ -437,7 +437,7 @@ public List<ModelState<?>> getAllModels() {
      */
     @Deprecated
     public void stopModel(String detectorId, String modelId) {
-        logger.info(String.format("Stopping detector %s model %s", detectorId, modelId));
+        logger.info(String.format(Locale.ROOT, "Stopping detector %s model %s", detectorId, modelId));
         stopModel(forests, modelId, this::toCheckpoint);
         stopModel(thresholds, modelId, this::toCheckpoint);
     }
@@ -458,7 +458,7 @@ private <T> void stopModel(Map<String, ModelState<T>> models, String modelId, Fu
      * @param listener onResponse is called with null when the operation is completed
      */
     public void stopModel(String detectorId, String modelId, ActionListener<Void> listener) {
-        logger.info(String.format("Stopping detector %s model %s", detectorId, modelId));
+        logger.info(String.format(Locale.ROOT, "Stopping detector %s model %s", detectorId, modelId));
         stopModel(
             forests,
             modelId,

diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/ModelPartitioner.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/ml/ModelPartitioner.java
@@ -16,6 +16,7 @@
 package com.amazon.opendistroforelasticsearch.ad.ml;
 
 import java.util.AbstractMap.SimpleImmutableEntry;
+import java.util.Locale;
 import java.util.Map.Entry;
 
 import org.apache.logging.log4j.LogManager;
@@ -133,7 +134,7 @@ public Entry<Integer, Integer> getPartitionedForestSizes(RandomCutForest forest,
      * @return ID for the RCF model partition
      */
     public String getRcfModelId(String detectorId, int partitionNumber) {
-        return String.format(RCF_MODEL_ID_PATTERN, detectorId, partitionNumber);
+        return String.format(Locale.ROOT, RCF_MODEL_ID_PATTERN, detectorId, partitionNumber);
     }
 
     /**
@@ -143,6 +144,6 @@ public String getRcfModelId(String detectorId, int partitionNumber) {
      * @return ID for the thresholding model
      */
     public String getThresholdModelId(String detectorId) {
-        return String.format(THRESHOLD_MODEL_ID_PATTERN, detectorId);
+        return String.format(Locale.ROOT, THRESHOLD_MODEL_ID_PATTERN, detectorId);
     }
 }
diff --git a/...va/com/amazon/opendistroforelasticsearch/ad/rest/RestSearchAnomalyDetectorInfoAction.java b/...va/com/amazon/opendistroforelasticsearch/ad/rest/RestSearchAnomalyDetectorInfoAction.java
@@ -73,7 +73,10 @@ public List<RestHandler.Route> routes() {
                     String.format(Locale.ROOT, "%s/%s", AnomalyDetectorPlugin.AD_BASE_DETECTORS_URI, COUNT)
                 ),
                 // get if a detector name exists with name
-                new RestHandler.Route(RestRequest.Method.GET, String.format("%s/%s", AnomalyDetectorPlugin.AD_BASE_DETECTORS_URI, MATCH))
+                new RestHandler.Route(
+                    RestRequest.Method.GET,
+                    String.format(Locale.ROOT, "%s/%s", AnomalyDetectorPlugin.AD_BASE_DETECTORS_URI, MATCH)
+                )
             );
     }
 }
diff --git a/.../amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java b/.../amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java
@@ -23,6 +23,7 @@
 import java.time.Instant;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.stream.Collectors;
 
@@ -391,13 +392,13 @@ private void validateCategoricalField(String detectorId) {
             }
 
             if (foundField == false) {
-                listener.onFailure(new IllegalArgumentException(String.format(NOT_FOUND_ERR_MSG, categoryField0)));
+                listener.onFailure(new IllegalArgumentException(String.format(Locale.ROOT, NOT_FOUND_ERR_MSG, categoryField0)));
                 return;
             }
 
             searchAdInputIndices(detectorId);
         }, error -> {
-            String message = String.format("Fail to get the index mapping of %s", anomalyDetector.getIndices());
+            String message = String.format(Locale.ROOT, "Fail to get the index mapping of %s", anomalyDetector.getIndices());
             logger.error(message, error);
             listener.onFailure(new IllegalArgumentException(message));
         });
@@ -459,6 +460,7 @@ private void onSearchADNameResponse(SearchResponse response, String detectorId,
         if (response.getHits().getTotalHits().value > 0) {
             String errorMsg = String
                 .format(
+                    Locale.ROOT,
                     "Cannot create anomaly detector with name [%s] as it's already used by detector %s",
                     name,
                     Arrays.stream(response.getHits().getHits()).map(hit -> hit.getId()).collect(Collectors.toList())

diff --git a/.../java/com/amazon/opendistroforelasticsearch/ad/transport/ADResultBulkTransportAction.java b/.../java/com/amazon/opendistroforelasticsearch/ad/transport/ADResultBulkTransportAction.java
@@ -20,6 +20,7 @@
 import static org.elasticsearch.index.IndexingPressure.MAX_INDEXING_BYTES;
 
 import java.io.IOException;
+import java.util.Locale;
 import java.util.Random;
 
 import org.apache.logging.log4j.LogManager;
@@ -121,7 +122,7 @@ private void addResult(BulkRequest bulkRequest, AnomalyResult result) {
             IndexRequest indexRequest = new IndexRequest(indexName).source(result.toXContent(builder, RestHandlerUtils.XCONTENT_WITH_TYPE));
             bulkRequest.add(indexRequest);
         } catch (IOException e) {
-            LOG.error(String.format("Failed to prepare bulk %s", indexName), e);
+            LOG.error(String.format(Locale.ROOT, "Failed to prepare bulk %s", indexName), e);
         }
     }
 }
diff --git a/...java/com/amazon/opendistroforelasticsearch/ad/transport/AnomalyResultTransportAction.java b/...java/com/amazon/opendistroforelasticsearch/ad/transport/AnomalyResultTransportAction.java
@@ -932,7 +932,7 @@ private boolean shouldStart(
         }
 
         if (stateManager.isMuted(thresholdNodeId)) {
-            listener.onFailure(new InternalFailure(adID, String.format(NODE_UNRESPONSIVE_ERR_MSG + " %s", thresholdModelID)));
+            listener.onFailure(new InternalFailure(adID, String.format(Locale.ROOT, NODE_UNRESPONSIVE_ERR_MSG + " %s", thresholdModelID)));
             return false;
         }
 
@@ -1060,7 +1060,7 @@ private Optional<AnomalyDetectionException> coldStartIfNoCheckPoint(AnomalyDetec
                 LOG.info("Trigger cold start for {}", detectorId);
                 coldStart(detector);
             } else {
-                String errorMsg = String.format("Fail to get checkpoint state for %s", detectorId);
+                String errorMsg = String.format(Locale.ROOT, "Fail to get checkpoint state for %s", detectorId);
                 LOG.error(errorMsg, exception);
                 stateManager.setLastColdStartException(detectorId, new AnomalyDetectionException(errorMsg, exception));
             }

diff --git a/...java/com/amazon/opendistroforelasticsearch/ad/transport/EntityProfileTransportAction.java b/...java/com/amazon/opendistroforelasticsearch/ad/transport/EntityProfileTransportAction.java
@@ -16,6 +16,7 @@
 package com.amazon.opendistroforelasticsearch.ad.transport;
 
 import java.io.IOException;
+import java.util.Locale;
 import java.util.Optional;
 import java.util.Set;
 
@@ -118,7 +119,7 @@ protected void doExecute(Task task, EntityProfileRequest request, ActionListener
             listener.onResponse(builder.build());
         } else {
             // redirect
-            LOG.debug("Sending RCF polling request to {} for detector {}, entity {}", nodeId, adID, entityValue);
+            LOG.debug("Sending entity profile request to {} for detector {}, entity {}", nodeId, adID, entityValue);
 
             try {
                 transportService
@@ -152,7 +153,7 @@ public String executor() {
                         }
                     );
             } catch (Exception e) {
-                LOG.error(String.format("Fail to get entity profile for detector {}, entity {}", adID, entityValue), e);
+                LOG.error(String.format(Locale.ROOT, "Fail to get entity profile for detector {}, entity {}", adID, entityValue), e);
                 listener.onFailure(new AnomalyDetectionException(adID, FAIL_TO_GET_ENTITY_PROFILE_MSG, e));
             }
 

diff --git a/...in/java/com/amazon/opendistroforelasticsearch/ad/transport/RCFPollingTransportAction.java b/...in/java/com/amazon/opendistroforelasticsearch/ad/transport/RCFPollingTransportAction.java
@@ -16,6 +16,7 @@
 package com.amazon.opendistroforelasticsearch.ad.transport;
 
 import java.io.IOException;
+import java.util.Locale;
 import java.util.Optional;
 
 import org.apache.logging.log4j.LogManager;
@@ -139,7 +140,7 @@ public String executor() {
 
                     });
             } catch (Exception e) {
-                LOG.error(String.format("Fail to poll RCF models for {}", adID), e);
+                LOG.error(String.format(Locale.ROOT, "Fail to poll RCF models for {}", adID), e);
                 listener.onFailure(new AnomalyDetectionException(adID, FAIL_TO_GET_RCF_UPDATE_MSG, e));
             }