[ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results #47050

benwtrent · 2019-09-24T21:13:09Z

This PR adds a new geo_results.* field for actual/typical fields in the anomaly record storage, and the anomaly cause storage.

I did not opt for these two items to be returned within the HLRC client and to have the HLRC have a helper method that transforms the two points to a GeoPoint if the function is lat_long.

The purpose behind this change is so that lan_long anomaly records can be trivially visible within Kibana Maps. Additionally, helpful queries and inspections can be made against the records as well with the plethora of geo capabilities within the stack.

elasticmachine · 2019-09-24T21:13:11Z

Pinging @elastic/ml-core

benwtrent · 2019-10-16T18:35:35Z

@elasticmachine update branch

…ords

…o-mapping-for-anomaly-records

benwtrent · 2019-10-25T12:18:02Z

@alexfrancoeur Thought you would enjoy this. This PR adds geo_point mappings for anomaly detection jobs. Just needed to add a index pattern to include the .ml-anomalies-*
and the results of a lat_long detector are displayable in maps

alexfrancoeur · 2019-10-25T14:09:49Z

@benwtrent your assumption was correct, I absolutely love this 😄❤️ ❤️ This type of functionality is the perfect way to bring in a streamlined approach for creating ML jobs from within the maps application. Ideally, our end users won't have to know anything about the .ml-anomalies-* index.

Now I'm curious, would be it possible to support geo_shape in the future?

cc: @elastic/kibana-gis

benwtrent · 2019-10-25T14:17:27Z

@alexfrancoeur the lat_long detector is the only detector that generates records that conform to geo typed data. And it only generates points. I opted for geo_point as it still seems to be the broader accepted and usable value across the stack (more agg/search support, etc.).

As for more detectors in the future that look at other geo data (shapes, lines, etc.) that is definitely within the realm of possibility, just not in the near future :)

alexfrancoeur · 2019-10-25T14:23:28Z

Thanks for the quick response! Agreed that points are more heavily used today. If we choose to introduce a streamlined UX for geo_point anomaly detection from Elastic Maps, it'll be interesting to see how many requests we get for geo_shape. geo_shape support definitely feels more like a roadmap feature, but thought I'd ask 😄

droberts195

The ease of getting ML lat_long anomalies onto maps is great.

My main comment is about whether the fields should be generated in the C++ code. I think we should ask for more opinions about this.

droberts195 · 2019-10-29T13:32:51Z

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

        builder.endObject();
        return builder;
    }

+    private void writeDoublesAsGeoPoint(XContentBuilder builder, String fieldName, List<Double> doubles) throws IOException {
+        if (doubles != null) {
+            //TODO should we fail if `doubles` is not an array of length 2?


I think an internal assertion as you've got it now is best. Throwing an exception here would mean it was impossible to get anything from the results endpoint.

However, see my main comment because that would render this a moot point as the whole method could be deleted.

.../plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobResultsPersister.java

…o-mapping-for-anomaly-records

droberts195

LGTM with the caveat that you should check with the ES core infra team about changing the method visibility in XContentBuilder. (If they don't like it there's an easy change that can be made to this PR to avoid it.)

droberts195 · 2019-11-08T13:56:38Z

libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

@@ -850,7 +850,7 @@ private XContentBuilder value(ToXContent value) throws IOException {
        return value(value, ToXContent.EMPTY_PARAMS);
    }

-    private XContentBuilder value(ToXContent value, ToXContent.Params params) throws IOException {
+    public XContentBuilder value(ToXContent value, ToXContent.Params params) throws IOException {


I don't really see why making this public would be a problem given what it does and the other public methods nearby. However, you should probably run this by somebody in the ES core infra team before merging.

There is a simple way to avoid making this public - see my next comment.

droberts195 · 2019-11-08T13:58:11Z

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

-            builder.field(CAUSES.getPreferredName(), causes);
+            builder.startArray(CAUSES.getPreferredName());
+            for (AnomalyCause cause : causes) {
+                builder.value(cause, params);


This line could be changed to cause.toXContent(builder, params); to avoid the need to change method visibility in the X-Content builder.

…o-mapping-for-anomaly-records

benwtrent · 2019-11-11T14:40:44Z

run elasticsearch-ci/2

…_long` results (elastic#47050) [ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results (elastic#47050) Related PR: elastic/ml-cpp#809

Adds new geo_point formatted fields for Java side consumption. These are only written if we have actual|typical values that have length of 2, and are using a lat_long function. This is my first PR against this repo, let me know if I missed something. related elastic/elasticsearch#47050

…r `lat_long` results (#47050) (#48958) * [ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results (#47050) [ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results (#47050) Related PR: elastic/ml-cpp#809 * adjusting bwc version

Adds new geo_point formatted fields for Java side consumption. These are only written if we have actual|typical values that have length of 2, and are using a lat_long function. This is my first PR against this repo, let me know if I missed something. related elastic/elasticsearch#47050

alexfrancoeur · 2019-11-12T01:12:03Z

wooo! Nice one @benwtrent

…_long` results (elastic#47050) [ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results (elastic#47050) Related PR: elastic/ml-cpp#809

[ML] Add new (actual|typical)_geo field for lat_lon results

b939b82

benwtrent added >enhancement :ml Machine learning v8.0.0 v7.5.0 labels Sep 24, 2019

Update AnomalyRecordTests.java

b05ba94

benwtrent added v7.6.0 and removed v7.5.0 labels Oct 16, 2019

elasticmachine and others added 2 commits October 16, 2019 11:35

Merge branch 'master' into feature/ml-add-geo-mapping-for-anomaly-rec…

89a23b6

…ords

Merge remote-tracking branch 'upstream/master' into feature/ml-add-ge…

5aa0589

…o-mapping-for-anomaly-records

benwtrent changed the title ~~[ML] Add new (actual|typical)_geo field for lat_lon results~~ [ML] Add new geo.(actual|typical) fields for lat_lon results Oct 25, 2019

changing from _geo, to geo.* format

1598c1f

benwtrent changed the title ~~[ML] Add new geo.(actual|typical) fields for lat_lon results~~ [ML] Add new geo_results.(actual|typical) fields for lat_lon results Oct 25, 2019

benwtrent added 2 commits October 25, 2019 11:42

changing geo field to geo_results

3db79b8

finish refactor

5fbba22

droberts195 reviewed Oct 29, 2019

View reviewed changes

benwtrent added 2 commits November 4, 2019 09:41

Merge remote-tracking branch 'upstream/master' into feature/ml-add-ge…

a71940b

…o-mapping-for-anomaly-records

moving to serializing from the C++ side

62b04c0

benwtrent mentioned this pull request Nov 6, 2019

[ML] add geo point formatting string for actual|typical elastic/ml-cpp#809

Merged

droberts195 approved these changes Nov 8, 2019

View reviewed changes

benwtrent added 2 commits November 11, 2019 08:50

fixing toxcontent interactions

6752b61

Merge remote-tracking branch 'upstream/master' into feature/ml-add-ge…

f4191b6

…o-mapping-for-anomaly-records

benwtrent removed the >enhancement label Nov 11, 2019

benwtrent added the >non-issue label Nov 11, 2019

benwtrent changed the title ~~[ML] Add new geo_results.(actual|typical) fields for lat_lon results~~ [ML] Add new geo_results.(actual_point|typical_point) fields for lat_lon results Nov 11, 2019

benwtrent changed the title ~~[ML] Add new geo_results.(actual_point|typical_point) fields for lat_lon results~~ [ML] Add new geo_results.(actual_point|typical_point) fields for lat_long results Nov 11, 2019

benwtrent merged commit ee8853f into elastic:master Nov 11, 2019

benwtrent deleted the feature/ml-add-geo-mapping-for-anomaly-records branch November 11, 2019 18:21

benwtrent mentioned this pull request Nov 11, 2019

[7.x] [ML] Add new geo_results.(actual_point|typical_point) fields for lat_long results (#47050) #48958

Merged

benwtrent mentioned this pull request Nov 11, 2019

[ML] add geo point formatting string for actual|typical (#809) elastic/ml-cpp#817

Merged

droberts195 mentioned this pull request Nov 12, 2019

[ML] record lat_lon detector results as geo_point #42862

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

This was referenced Nov 1, 2022

[DOCS] Clarify description of geo_results #91237

Merged

Add geo_results.actual_point and typical_point elastic/elasticsearch-specification#1984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results #47050

[ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results #47050

benwtrent commented Sep 24, 2019 •

edited

Loading

elasticmachine commented Sep 24, 2019

benwtrent commented Oct 16, 2019

benwtrent commented Oct 25, 2019

alexfrancoeur commented Oct 25, 2019

benwtrent commented Oct 25, 2019

alexfrancoeur commented Oct 25, 2019

droberts195 left a comment

droberts195 Oct 29, 2019

droberts195 left a comment

droberts195 Nov 8, 2019

droberts195 Nov 8, 2019

benwtrent commented Nov 11, 2019

alexfrancoeur commented Nov 12, 2019

[ML] Add new geo_results.(actual_point|typical_point) fields for lat_long results #47050

[ML] Add new geo_results.(actual_point|typical_point) fields for lat_long results #47050

Conversation

benwtrent commented Sep 24, 2019 • edited Loading

elasticmachine commented Sep 24, 2019

benwtrent commented Oct 16, 2019

benwtrent commented Oct 25, 2019

alexfrancoeur commented Oct 25, 2019

benwtrent commented Oct 25, 2019

alexfrancoeur commented Oct 25, 2019

droberts195 left a comment

Choose a reason for hiding this comment

droberts195 Oct 29, 2019

Choose a reason for hiding this comment

droberts195 left a comment

Choose a reason for hiding this comment

droberts195 Nov 8, 2019

Choose a reason for hiding this comment

droberts195 Nov 8, 2019

Choose a reason for hiding this comment

benwtrent commented Nov 11, 2019

alexfrancoeur commented Nov 12, 2019

[ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results #47050

[ML] Add new geo_results.(actual_point|typical_point) fields for `lat_long` results #47050

benwtrent commented Sep 24, 2019 •

edited

Loading