Remove types from Monitoring plugin "backend" code #37745

ycombinator · 2019-01-23T03:05:02Z

This PR removes the use of document types from the monitoring exporters and template + watches setup code.

It does not remove the notion of types from the monitoring bulk API endpoint "front end" code as that code will eventually just go away in 8.0 and be replaced with Beats as collectors/shippers directly to the monitoring cluster.

elasticmachine · 2019-01-23T03:05:19Z

Pinging @elastic/es-core-features

...ck/plugin/monitoring/src/main/resources/monitoring/watches/elasticsearch_cluster_status.json

...onitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExportBulk.java

gwbrown · 2019-01-23T19:56:06Z

It looks like this may relate to #37442 - does this remove the types from the index templates for the monitoring indices, or just stop using them in the code? It looks like the latter.

ycombinator · 2019-01-26T15:00:45Z

@gwbrown Thanks for linking to that issue. As it stands, this PR just removes types from the code and some watch definitions used by Monitoring. I'm happy to add the template changes to this PR as well.

martijnvg

Hey @ycombinator can you merge master into this PR? There are some test failures. (for example tests expecting a warning about type being used while that is no longer the case and malformed watches / index templates)

martijnvg · 2019-01-29T08:19:24Z

x-pack/plugin/monitoring/src/main/resources/monitoring/watches/elasticsearch_nodes.json

@@ -158,7 +158,6 @@
    "add_to_alerts_index": {
      "index": {
        "index": ".monitoring-alerts-6",


, needs to be removed, otherwise this file can't be parsed

Incidentally, @pickypg, do you recall why this particular watch doesn't set a document ID whereas all the other cluster alerts watches do?

@ycombinator It was briefly before the Task Manager stuff began in Kibana (and partly what led to it), but because you could restart multiple nodes in sequence and you didn't want to lose the history of all of them. The goal was if this index ever became a burden to just delete it and recreate it (anecdotally I've never seen the index with >500 documents) or move to index rollover.

.../java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterResourceIntegTests.java

ycombinator · 2019-01-31T19:00:01Z

CI failures are legitimate (related to this PR) and I'm able to reproduce them locally. Fixing...

ycombinator · 2019-02-01T04:45:19Z

@pickypg @martijnvg I’ve managed to make most of the tests pass. However I could use some help/guidance with the 2 monitoring unit tests that are still failing. They both fail with the same error:

java.lang.IllegalArgumentException: Malformed [mappings] section for type [enabled], should include an inner object describing the mapping

As far as I can tell, the test template is structured correctly. If I try to manually create a template with the same structure in Console, it is accepted as valid. So I’m not sure why this error is being thrown.

I’m probably overlooking something simple so I could use a second/third set of eyes here, please. Thank you!

martijnvg · 2019-02-01T08:47:28Z

@ycombinator I've pushed a commit that fixes the tests. enabled need to be wrapped in _source object. Also when using PutIndexTemplateRequest#setSource(...) the special _doc type still needs to be used. Only the RestPutIndexTemplateAction has special logic, that if _doc is missing, it adds that. This does not happen when using the request object, so here in this test we need manually add it.

jtibshirani · 2019-02-01T22:32:07Z

@ycombinator @gwbrown I've started a discussion offline about whether we should update the monitoring templates to be typeless as opposed to using the doc type as they do now. The main concerns around making the change are that is will be a breaking change for some consumers. I don't think that discussion needs to hold up this PR though, as it is already a huge step forward in terms of reducing deprecation messages.

ycombinator · 2019-02-02T10:56:29Z

.../java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterResourceIntegTests.java

@@ -81,11 +82,14 @@ private static BytesReference generateTemplateSource(final String name, final In
                .field("index.number_of_replicas", 0)
            .endObject()
            .startObject("mappings")
-                .startObject("doc")
+                // Still need use type, RestPutIndexTemplateAction#prepareRequestSource has logic that adds type if missing
+                .startObject(MapperService.SINGLE_MAPPING_NAME)


@jtibshirani In your offline discussion you mentioned:

Internal elasticsearch products like monitoring and security have been using 'doc' as a type name, and in particular create index templates using the custom type 'doc'.

Given that, should the argument to startObject here be MapperService.SINGLE_MAPPING_NAME, which would evaluate to _doc or should it be "doc"?

I think for now we should keep it the same as before (using doc), until we come to a firm conclusion in that discussion to update the monitoring templates.

ycombinator · 2019-02-02T12:37:01Z

@pickypg @jtibshirani @martijnvg @jakelandis @gwbrown I think this PR is ready for another round of review now. Thanks!

jakelandis · 2019-02-04T16:22:36Z

...in/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java

@@ -127,7 +126,7 @@ public void testMonitoringBulk() throws Exception {

            final MonitoringBulkResponse bulkResponse =
                    new MonitoringBulkRequestBuilder(client())
-                            .add(system, null, new BytesArray(createBulkEntity().getBytes("UTF-8")), XContentType.JSON,
+                            .add(system, "monitoring_data_type", new BytesArray(createBulkEntity().getBytes("UTF-8")), XContentType.JSON,


Why add in a default type here ?

@pickypg can correct me if I'm wrong but I believe this argument is the monitoring data type, e.g. beats_stats or logstash_state, etc. that are sent in to the POST _xpack/monitoring/_bulk API. So I wanted to be more explicit about what the type is, so we don't confuse it in the future with _type.

They are. We use that as a mechanism for routing the document to the right index.

thanks, i see now.

pickypg

LGTM

martijnvg

LGTM

jakelandis

LGTM, thanks for doing this !

…-lease-expiration * elastic/master: (24 commits) Add support for API keys to access Elasticsearch (elastic#38291) Add typless client side GetIndexRequest calls and response class (elastic#37778) Limit token expiry to 1 hour maximum (elastic#38244) add docs saying mixed-cluster ILM is not supported (elastic#37954) Skip unsupported languages for tests (elastic#38328) Deprecate `_type` in simulate pipeline requests (elastic#37949) Mute testCannotShrinkLeaderIndex (elastic#38374) Tighten mapping syncing in ccr remote restore (elastic#38071) Add test for `PutFollowAction` on a closed index (elastic#38236) Fix SSLContext pinning to TLSV1.2 in reload tests (elastic#38341) Mute RareClusterStateIT.testDelayedMappingPropagationOnReplica (elastic#38357) Deprecate types in rollover index API (elastic#38039) Types removal - fix FullClusterRestartIT warning expectations (elastic#38310) Fix ILM explain response to allow unknown fields (elastic#38054) Mute testFollowIndexAndCloseNode (elastic#38360) Docs: Drop inline callout from scroll example (elastic#38340) Deprecate HLRC security methods (elastic#37883) Remove types from Monitoring plugin "backend" code (elastic#37745) Add Composite to AggregationBuilders (elastic#38207) Clarify slow cluster-state log messages (elastic#38302) ...

ycombinator added v7.0.0 :Data Management/Monitoring labels Jan 23, 2019

ycombinator requested a review from pickypg January 23, 2019 03:05

ycombinator commented Jan 23, 2019

View reviewed changes

...ck/plugin/monitoring/src/main/resources/monitoring/watches/elasticsearch_cluster_status.json Show resolved Hide resolved

ycombinator commented Jan 23, 2019

View reviewed changes

...onitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExportBulk.java Outdated Show resolved Hide resolved

jakelandis requested a review from martijnvg January 28, 2019 15:35

martijnvg reviewed Jan 29, 2019

View reviewed changes

ycombinator force-pushed the monitoring-remove-type branch 3 times, most recently from 5a225be to ce9b502 Compare January 31, 2019 16:42

ycombinator force-pushed the monitoring-remove-type branch from 5ab0b23 to 2c754fa Compare February 1, 2019 03:00

ycombinator changed the title ~~Remove types from Monitoring plugin code~~ Remove types from Monitoring plugin "backend" code Feb 1, 2019

ycombinator added 10 commits February 1, 2019 14:49

Remove types from Monitoring plugin code

2cd4b9b

Adding monitoring data types to trace logs

d824964

Make JSON Valid Again

9d00b79

Fixing bad field nesting

f691585

Fixing checkstyle violation

da1a299

Some unit test fixes (more coming...)

abd8699

More unit test fixes

33fe139

Fixing checkstyle violations

669f294

Template mapping fixes

b7eaeb8

Reverting change to monitoring bulk endpoint code and tests

ab9c7ae

ycombinator force-pushed the monitoring-remove-type branch from b222d22 to ab9c7ae Compare February 1, 2019 22:49

Adding back doc to template mappings

8876abe

ycombinator force-pushed the monitoring-remove-type branch from ed06bac to 8876abe Compare February 2, 2019 00:20

martijnvg and others added 2 commits February 2, 2019 11:07

fixed tests

9bcc095

Clarify type in the context of this test

df335e3

ycombinator force-pushed the monitoring-remove-type branch from 496f703 to df335e3 Compare February 2, 2019 10:42

ycombinator commented Feb 2, 2019

View reviewed changes

ycombinator added 2 commits February 2, 2019 18:27

Fixing _type

8f41a4b

Removing unused import

79e7eed

jakelandis reviewed Feb 4, 2019

View reviewed changes

pickypg approved these changes Feb 4, 2019

View reviewed changes

Reverting unnecessary change

c6d1c10

martijnvg approved these changes Feb 4, 2019

View reviewed changes

ycombinator merged commit be1bb0e into elastic:master Feb 4, 2019

ycombinator deleted the monitoring-remove-type branch February 4, 2019 18:58

jakelandis reviewed Feb 4, 2019

View reviewed changes

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

jimczi added the >non-issue label Feb 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove types from Monitoring plugin "backend" code #37745

Remove types from Monitoring plugin "backend" code #37745

ycombinator commented Jan 23, 2019 •

edited

Loading

elasticmachine commented Jan 23, 2019

gwbrown commented Jan 23, 2019

ycombinator commented Jan 26, 2019

martijnvg left a comment

martijnvg Jan 29, 2019

ycombinator Jan 29, 2019

pickypg Jan 29, 2019

ycombinator commented Jan 31, 2019 •

edited

Loading

ycombinator commented Feb 1, 2019

martijnvg commented Feb 1, 2019

jtibshirani commented Feb 1, 2019

ycombinator Feb 2, 2019

jtibshirani Feb 3, 2019 •

edited

Loading

ycombinator commented Feb 2, 2019

jakelandis Feb 4, 2019

ycombinator Feb 4, 2019

pickypg Feb 4, 2019

jakelandis Feb 4, 2019

pickypg left a comment

martijnvg left a comment

jakelandis left a comment

Remove types from Monitoring plugin "backend" code #37745

Remove types from Monitoring plugin "backend" code #37745

Conversation

ycombinator commented Jan 23, 2019 • edited Loading

elasticmachine commented Jan 23, 2019

gwbrown commented Jan 23, 2019

ycombinator commented Jan 26, 2019

martijnvg left a 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

ycombinator commented Jan 31, 2019 • edited Loading

ycombinator commented Feb 1, 2019

martijnvg commented Feb 1, 2019

jtibshirani commented Feb 1, 2019

Choose a reason for hiding this comment

jtibshirani Feb 3, 2019 • edited Loading

Choose a reason for hiding this comment

ycombinator commented Feb 2, 2019

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

pickypg left a comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

jakelandis left a comment

Choose a reason for hiding this comment

ycombinator commented Jan 23, 2019 •

edited

Loading

ycombinator commented Jan 31, 2019 •

edited

Loading

jtibshirani Feb 3, 2019 •

edited

Loading