[ML] Label anomalies with multi_bucket_impact by edsavage · Pull Request #34233 · elastic/elasticsearch

edsavage · 2018-10-02T15:14:05Z

Add the multi_bucket_impact field to record results.

elasticmachine · 2018-10-02T15:14:07Z

Pinging @elastic/ml-core

droberts195

I suggested a renaming change but I think IntelliJ's refactoring functionality (right click -> refactor -> rename) should make light work of it. It has the option to automatically rename arguments and accessors.

droberts195 · 2018-10-02T15:54:10Z

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/results/AnomalyRecord.java

     * Result fields (all detector types)
     */
    public static final ParseField PROBABILITY = new ParseField("probability");
+    public static final ParseField IMPACT = new ParseField("multi_bucket_impact");


I think it would be more future proof if the field was MULTI_BUCKET_IMPACT rather than just IMPACT. (You never know if we might need to add some other type of impact in the future.)

Same in the server-side file.

droberts195 · 2018-10-02T15:56:18Z

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/results/AnomalyRecord.java

    private final String jobId;
    private int detectorIndex;
    private double probability;
+    private double impact;


Similar to above, I think this should be called multiBucketImpact, and the accessors should also be renamed to match.

Also, I think it should be an object of type Double so that it can be null. If people get results from old versions the field won't exist and having it as a double will set it to 0.0 in these cases.

Same in the server-side file.

droberts195 · 2018-10-02T15:57:06Z

client/rest-high-level/src/main/java/org/elasticsearch/client/ml/job/results/AnomalyRecord.java

            && this.detectorIndex == that.detectorIndex
            && this.bucketSpan == that.bucketSpan
            && this.probability == that.probability
+            && this.impact == that.impact


This will need changing to Objects.equals() for type Double.

Same in the server-side file.

droberts195 · 2018-10-02T15:59:27Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/results/AnomalyRecord.java

        jobId = in.readString();
        detectorIndex = in.readInt();
        probability = in.readDouble();
+        impact = in.readDouble();


To make this backwards compatible it needs to be:

if (in.version().onOrAfter(Version.V_6_5_0)) { impact = in.readOptionalDouble(); }

droberts195 · 2018-10-02T16:00:21Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/results/AnomalyRecord.java

        out.writeString(jobId);
        out.writeInt(detectorIndex);
        out.writeDouble(probability);
+        out.writeDouble(impact);


To make this backwards compatible it needs to be:

if (out.version().onOrAfter(Version.V_6_5_0)) { out.writeOptionalDouble(impact); }

droberts195 · 2018-10-02T16:01:21Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/results/AnomalyRecord.java

        builder.field(Job.ID.getPreferredName(), jobId);
        builder.field(Result.RESULT_TYPE.getPreferredName(), RESULT_TYPE_VALUE);
        builder.field(PROBABILITY.getPreferredName(), probability);
+        builder.field(IMPACT.getPreferredName(), impact);


When the type is changed to Double this should be wrapped in a null check so we write nothing if it's null.

droberts195 · 2018-10-02T16:05:21Z

...est-high-level/src/test/java/org/elasticsearch/client/ml/job/results/AnomalyRecordTests.java

        anomalyRecord.setActual(Collections.singletonList(randomDouble()));
        anomalyRecord.setTypical(Collections.singletonList(randomDouble()));
        anomalyRecord.setProbability(randomDouble());
+        anomalyRecord.setImpact(randomDouble());


To confirm null works once the type is changed to Double, surround this line with if (randomBoolean()) {.

Same for the server-side file.

Will do - thanks for the quick review @droberts195

droberts195

LGTM as long as the CI goes green.

If there's a failure it's most likely related to the BWC aspects.

…y_labelling

* [ML] Label anomalies with multi_bucket_impact Add the multi_bucket_impact field to record results.

* master: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* rename-ccr-stats: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* [ML] Label anomalies with multi_bucket_impact Add the multi_bucket_impact field to record results.

[ML] Label anomalies with multi_bucket_impact

de1cb60

Add the multi_bucket_impact field to record results.

edsavage added review v7.0.0 :ml Machine learning v6.5.0 labels Oct 2, 2018

edsavage mentioned this pull request Oct 2, 2018

[ML] Ability to label anomalies elastic/ml-cpp#197

Closed

droberts195 reviewed Oct 2, 2018

View reviewed changes

Attending to code review comments

aa92994

droberts195 approved these changes Oct 3, 2018

View reviewed changes

edsavage added 2 commits October 3, 2018 13:25

Merge branch 'master' of github.com:elastic/elasticsearch into anomal…

c0b6acc

…y_labelling

Merge branch 'master' of github.com:elastic/elasticsearch into anomal…

b1b7534

…y_labelling

edsavage merged commit 577261e into elastic:master Oct 4, 2018

edsavage added a commit that referenced this pull request Oct 4, 2018

[ML] Label anomalies with multi_bucket_impact (#34233)

f1799b8

* [ML] Label anomalies with multi_bucket_impact Add the multi_bucket_impact field to record results.

edsavage deleted the anomaly_labelling branch October 5, 2018 09:00

colings86 added the >feature label Oct 25, 2018

kcm pushed a commit that referenced this pull request Oct 30, 2018

[ML] Label anomalies with multi_bucket_impact (#34233)

a977d8e

* [ML] Label anomalies with multi_bucket_impact Add the multi_bucket_impact field to record results.

lcawl mentioned this pull request Nov 12, 2018

[DOCS] Adds release highlight for multi-bucket analysis #35469

Merged

lcawl mentioned this pull request Nov 29, 2018

[DOCS] Adds multi-bucket analysis to bucket overview elastic/stack-docs#158

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Label anomalies with multi_bucket_impact#34233

[ML] Label anomalies with multi_bucket_impact#34233
edsavage merged 4 commits intoelastic:masterfrom
edsavage:anomaly_labelling

edsavage commented Oct 2, 2018

Uh oh!

elasticmachine commented Oct 2, 2018

Uh oh!

droberts195 left a comment

Uh oh!

droberts195 Oct 2, 2018

Uh oh!

droberts195 Oct 2, 2018

Uh oh!

droberts195 Oct 2, 2018

Uh oh!

droberts195 Oct 2, 2018

Uh oh!

droberts195 Oct 2, 2018

Uh oh!

droberts195 Oct 2, 2018

Uh oh!

droberts195 Oct 2, 2018

Uh oh!

edsavage Oct 2, 2018

Uh oh!

droberts195 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

edsavage commented Oct 2, 2018

Uh oh!

elasticmachine commented Oct 2, 2018

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants