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

kaituo · 2021-01-27T20:12:12Z

Issue #, if available:

Description of changes:

This PR fixes incorrect or unnecessary log messages. The PR also uses ASCII to convert the init progress to a string. Otherwise, we may get unexpected characters.

Testing done:

gradle build

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2021-01-27T20:14:10Z

Codecov Report

Merging #374 (f0aeb7d) into master (16631c8) will decrease coverage by 0.00%.
The diff coverage is 53.19%.

@@             Coverage Diff              @@
##             master     #374      +/-   ##
============================================
- Coverage     78.95%   78.94%   -0.01%     
  Complexity     2655     2655              
============================================
  Files           247      247              
  Lines         11719    11723       +4     
  Branches       1009     1009              
============================================
+ Hits           9253     9255       +2     
- Misses         1991     1993       +2     
  Partials        475      475

Flag	Coverage Δ	Complexity Δ
plugin	`78.92% <53.19%> (-0.02%)`	`0.00 <9.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...stroforelasticsearch/ad/AnomalyDetectorRunner.java	`46.25% <0.00%> (ø)`	`7.00 <0.00> (ø)`
...distroforelasticsearch/ad/EntityProfileRunner.java	`78.16% <0.00%> (-1.12%)`	`31.00 <0.00> (ø)`
...n/opendistroforelasticsearch/ad/MemoryTracker.java	`77.33% <ø> (ø)`	`21.00 <0.00> (ø)`
...est/handler/IndexAnomalyDetectorActionHandler.java	`49.19% <0.00%> (ø)`	`32.00 <0.00> (ø)`
...arch/ad/transport/ADResultBulkTransportAction.java	`92.50% <0.00%> (ø)`	`13.00 <0.00> (ø)`
...search/ad/transport/RCFPollingTransportAction.java	`90.24% <0.00%> (ø)`	`6.00 <0.00> (ø)`
...ad/transport/handler/MultiEntityResultHandler.java	`23.25% <0.00%> (ø)`	`3.00 <0.00> (ø)`
...pendistroforelasticsearch/ad/ml/CheckpointDao.java	`83.76% <50.00%> (ø)`	`57.00 <0.00> (ø)`
...rch/ad/transport/AnomalyResultTransportAction.java	`80.78% <50.00%> (ø)`	`77.00 <0.00> (ø)`
...rch/ad/transport/EntityProfileTransportAction.java	`90.56% <50.00%> (ø)`	`8.00 <0.00> (ø)`
... and 15 more

ylwu-amzn · 2021-01-27T20:25:55Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/AbstractProfileRunner.java

-            String.format("%.0f%%", percent),
+            // Without Locale.US, sometimes conversions use localized decimal digits
+            // rather than the usual ASCII digits. See https://tinyurl.com/y5sdr5tp
+            String.format(Locale.US, "%.0f%%", percent),


Can we change other places for String.format ? I see such warning in build log

2021-01-26T23:00:07.9472702Z Forbidden method invocation: java.lang.String#format(java.lang.String,java.lang.Object[]) [Uses default locale] 2021-01-26T23:00:07.9475976Z in com.amazon.opendistroforelasticsearch.ad.transport.AnomalyResultTransportAction (AnomalyResultTransportAction.java:1063)

added missing locales in src/main

Do we need such Locale.US for other places of String.format()?
If only specifying Locale.US here, but nowhere else, can we get expected percentage string in US Locale?

Do we need such Locale.US for other places of String.format()?

Using String.format without locale is a forbidden pattern by ES. Apparently it is bad.

If only specifying Locale.US here, but nowhere else, can we get expected percentage string in US Locale?

Yes.

Using String.format without locale is a forbidden pattern by ES. Apparently it is bad.

Is there any documentation regarding why it is forbidden? want to learn more about it. In this case, I guess we should have a utility static method called StringFormat(), which is a wrapper of String.format(Locale.US, "", ""), so that we should only use the wrapper method StringFormat in this repo.

I guess you have already found the one in another conversation. For all forbidden APIs, check https://github.com/policeman-tools/forbidden-apis

There are too many cases like this. We cannot create a wrapper for all of them. I enabled locale check for string.format. build will fail if people forget about it.

sounds good to me.

ylwu-amzn · 2021-01-28T01:54:58Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/feature/SearchFeatureDao.java

@@ -511,7 +512,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.US, "Getting features for detector %s ending at %d", detector.getDetectorId(), endTime));


No much experience on Locale. Checked other plugin, looks they are using Locale.ROOT https://github.com/opendistro-for-elasticsearch/alerting/blob/master/core/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/core/resthandler/RestScheduledJobStatsHandler.kt#L81

From java doc:

The root locale is the locale whose language, country, and variant are empty ("") strings. This is regarded as the base locale of all locales, and is used as the language/country neutral locale for the locale sensitive operations.

is used as the language/country neutral locale

I think ROOT is a safer solution, but I am ok with either of US/ROOT/ENGLISH option. Feel free to use one of them.

Passing a specific locale here helps in most cases. If you are writing a GUI in English language, pass Locale.ENGLISH everywhere, otherwise text output of numbers or dates may not match the language of your GUI! If you want Formatter to behave in a invariant way, use Locale.ROOT, too (then it will for sure format numbers with period and no comma for thousands, just like Float.toString(float) does).

From https://dzone.com/articles/policemans-horror-default

I have no strong preference on either. Tested Locale.US fixes the init progress error. Locale.ROOT should work too.

I tested using 3 locales. They sure have differences.

jshell> System.out.println(DateFormat.getDateInstance(DateFormat.SHORT, Locale.US).format(new Date()));
1/28/21

jshell> System.out.println(DateFormat.getDateInstance(DateFormat.SHORT, Locale.ROOT).format(new Date()));
2021-01-28

jshell> System.out.println(DateFormat.getDateInstance(DateFormat.SHORT, Locale.ENGLISH).format(new Date()));
1/28/21

Hmmm, looks Locale.ROOT is also friendly to user in countries other than US. Let's use ROOT then. Thanks.

Replaced US with ROOT.

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

* 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

kaituo requested review from ylwu-amzn and yizheliu-amazon January 27, 2021 20:12

ylwu-amzn reviewed Jan 27, 2021

View reviewed changes

kaituo force-pushed the earlyReturnProfile3 branch from 3fd39a1 to 4ad4706 Compare January 27, 2021 20:26

kaituo added the bug Something isn't working label Jan 27, 2021

ylwu-amzn approved these changes Jan 27, 2021

View reviewed changes

ylwu-amzn reviewed Jan 28, 2021

View reviewed changes

kaituo added 3 commits January 28, 2021 15:16

Fix log messages and init progress for the profile API

a2caf3f

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

Add missing Locale in String.format

faaf1d8

enforce locale check in string.format

bb09144

kaituo force-pushed the earlyReturnProfile3 branch from 5201c71 to bb09144 Compare January 28, 2021 23:23

yizheliu-amazon approved these changes Jan 28, 2021

View reviewed changes

Replace Locale.US with Locale.ROOT

f0aeb7d

kaituo merged commit b40e12a into opendistro-for-elasticsearch:master Jan 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kaituo commented Jan 27, 2021 •

edited

Loading

codecov bot commented Jan 27, 2021 •

edited

Loading

ylwu-amzn Jan 27, 2021

kaituo Jan 27, 2021

yizheliu-amazon Jan 27, 2021

kaituo Jan 28, 2021 •

edited

Loading

yizheliu-amazon Jan 28, 2021

kaituo Jan 28, 2021

yizheliu-amazon Jan 28, 2021

ylwu-amzn Jan 28, 2021

ylwu-amzn Jan 28, 2021

yizheliu-amazon Jan 28, 2021

kaituo Jan 28, 2021

yizheliu-amazon Jan 28, 2021

kaituo Jan 29, 2021

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

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

Conversation

kaituo commented Jan 27, 2021 • edited Loading

codecov bot commented Jan 27, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaituo Jan 28, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaituo commented Jan 27, 2021 •

edited

Loading

codecov bot commented Jan 27, 2021 •

edited

Loading

kaituo Jan 28, 2021 •

edited

Loading