Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove types from Monitoring plugin "backend" code #37745

Merged
merged 16 commits into from
Feb 4, 2019
Merged

Remove types from Monitoring plugin "backend" code #37745

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2cd4b9b
Remove types from Monitoring plugin code
ycombinator Jan 23, 2019
d824964
Adding monitoring data types to trace logs
ycombinator Jan 29, 2019
9d00b79
Make JSON Valid Again
ycombinator Jan 29, 2019
f691585
Fixing bad field nesting
ycombinator Jan 29, 2019
da1a299
Fixing checkstyle violation
ycombinator Jan 30, 2019
abd8699
Some unit test fixes (more coming...)
ycombinator Jan 31, 2019
33fe139
More unit test fixes
ycombinator Jan 31, 2019
669f294
Fixing checkstyle violations
ycombinator Jan 31, 2019
b7eaeb8
Template mapping fixes
ycombinator Feb 1, 2019
ab9c7ae
Reverting change to monitoring bulk endpoint code and tests
ycombinator Feb 1, 2019
8876abe
Adding back doc to template mappings
ycombinator Feb 2, 2019
9bcc095
fixed tests
martijnvg Feb 1, 2019
df335e3
Clarify type in the context of this test
ycombinator Feb 2, 2019
8f41a4b
Fixing _type
ycombinator Feb 3, 2019
79e7eed
Removing unused import
ycombinator Feb 3, 2019
c6d1c10
Reverting unnecessary change
ycombinator Feb 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ...toring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExportBulk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ private byte[] toBulkBytes(final MonitoringDoc doc) throws IOException {
builder.startObject("index");
{
builder.field("_index", index);
builder.field("_type", "doc");
if (id != null) {
builder.field("_id", id);
}
Expand All @@ -162,7 +161,10 @@ private byte[] toBulkBytes(final MonitoringDoc doc) throws IOException {
// Adds final bulk separator
out.write(xContent.streamSeparator());

logger.trace("added index request [index={}, type={}, id={}]", index, doc.getType(), id);
logger.trace(
"http exporter [{}] - added index request [index={}, id={}, monitoring data type={}]",
name, index, id, doc.getType()
);

return BytesReference.toBytes(out.bytes());
} catch (Exception e) {
Expand Down
6 changes: 3 additions & 3 deletions ...monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalBulk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public void doAdd(Collection<MonitoringDoc> docs) throws ExportException {
try {
final String index = MonitoringTemplateUtils.indexName(formatter, doc.getSystem(), doc.getTimestamp());

final IndexRequest request = new IndexRequest(index, "doc");
final IndexRequest request = new IndexRequest(index);
if (Strings.hasText(doc.getId())) {
request.id(doc.getId());
}
Expand All @@ -82,8 +82,8 @@ public void doAdd(Collection<MonitoringDoc> docs) throws ExportException {
requestBuilder.add(request);

if (logger.isTraceEnabled()) {
logger.trace("local exporter [{}] - added index request [index={}, type={}, id={}, pipeline={}]",
name, request.index(), request.type(), request.id(), request.getPipeline());
logger.trace("local exporter [{}] - added index request [index={}, id={}, pipeline={}, monitoring data type={}]",
name, request.index(), request.id(), request.getPipeline(), doc.getType());
}
} catch (Exception e) {
if (exception == null) {
Expand Down
1 change: 0 additions & 1 deletion ...plugin/monitoring/src/main/resources/monitoring/watches/elasticsearch_cluster_status.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@
"add_to_alerts_index": {
"index": {
"index": ".monitoring-alerts-6",
"doc_type": "doc",
ycombinator marked this conversation as resolved.
Show resolved Hide resolved
"doc_id": "${monitoring.watch.unique_id}"
}
},
Expand Down
3 changes: 1 addition & 2 deletions x-pack/plugin/monitoring/src/main/resources/monitoring/watches/elasticsearch_nodes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,7 @@
"actions": {
"add_to_alerts_index": {
"index": {
"index": ".monitoring-alerts-6",
"doc_type": "doc"
"index": ".monitoring-alerts-6"
}
},
"send_email_to_admin": {
Expand Down
1 change: 0 additions & 1 deletion ...ugin/monitoring/src/main/resources/monitoring/watches/elasticsearch_version_mismatch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@
"add_to_alerts_index": {
"index": {
"index": ".monitoring-alerts-6",
"doc_type": "doc",
"doc_id": "${monitoring.watch.unique_id}"
}
},
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugin/monitoring/src/main/resources/monitoring/watches/kibana_version_mismatch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@
"add_to_alerts_index": {
"index": {
"index": ".monitoring-alerts-6",
"doc_type": "doc",
"doc_id": "${monitoring.watch.unique_id}"
}
},
Expand Down
1 change: 0 additions & 1 deletion ...ck/plugin/monitoring/src/main/resources/monitoring/watches/logstash_version_mismatch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@
"add_to_alerts_index": {
"index": {
"index": ".monitoring-alerts-6",
"doc_type": "doc",
"doc_id": "${monitoring.watch.unique_id}"
}
},
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugin/monitoring/src/main/resources/monitoring/watches/xpack_license_expiration.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@
"add_to_alerts_index": {
"index": {
"index": ".monitoring-alerts-6",
"doc_type": "doc",
"doc_id": "${monitoring.watch.unique_id}"
}
},
Expand Down
1 change: 0 additions & 1 deletion ...g/src/test/java/org/elasticsearch/xpack/monitoring/action/MonitoringBulkRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ public void testAddRequestContentWithUnrecognizedIndexName() throws IOException
assertThat(e.getMessage(), containsString("unrecognized index name [" + indexName + "]"));
//This test's JSON contains outdated references to types
assertWarnings(RestBulkAction.TYPES_DEPRECATION_MESSAGE);

}

public void testSerialization() throws IOException {
Expand Down
1 change: 0 additions & 1 deletion .../test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ private void checkMonitoringDocs() {
assertTrue("document is missing cluster_uuid field", Strings.hasText((String) source.get("cluster_uuid")));
assertTrue("document is missing timestamp field", Strings.hasText(timestamp));
assertTrue("document is missing type field", Strings.hasText(type));
assertEquals("document _type is 'doc'", "doc", hit.getType());

@SuppressWarnings("unchecked")
Map<String, Object> docSource = (Map<String, Object>) source.get("doc");
Expand Down
10 changes: 7 additions & 3 deletions ...va/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterResourceIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.ingest.PipelineConfiguration;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.xpack.core.monitoring.MonitoredSystem;
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"?

Copy link
Contributor

@jtibshirani jtibshirani Feb 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.startObject("_source")
.field("enabled", false)
.endObject()
.startObject("_meta")
.field("test", true)
.endObject()
.field("enabled", false)
.endObject()
.endObject();

Expand Down Expand Up @@ -193,7 +197,7 @@ private void assertTemplateNotUpdated() {
final String name = MonitoringTemplateUtils.templateName(system.getSystem());

for (IndexTemplateMetaData template : client().admin().indices().prepareGetTemplates(name).get().getIndexTemplates()) {
final String docMapping = template.getMappings().get("doc").toString();
final String docMapping = template.getMappings().get(MapperService.SINGLE_MAPPING_NAME).toString();

assertThat(docMapping, notNullValue());
assertThat(docMapping, containsString("test"));
Expand Down
13 changes: 5 additions & 8 deletions ...monitoring/src/test/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.elasticsearch.license.License;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.rest.action.document.RestBulkAction;
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.search.SearchHits;
import org.elasticsearch.search.collapse.CollapseBuilder;
Expand Down Expand Up @@ -104,11 +103,11 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
}

private String createBulkEntity() {
return "{\"index\":{\"_type\":\"test\"}}\n" +
return "{\"index\":{}}\n" +
"{\"foo\":{\"bar\":0}}\n" +
"{\"index\":{\"_type\":\"test\"}}\n" +
"{\"index\":{}}\n" +
"{\"foo\":{\"bar\":1}}\n" +
"{\"index\":{\"_type\":\"test\"}}\n" +
"{\"index\":{}}\n" +
"{\"foo\":{\"bar\":2}}\n" +
"\n";
}
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add in a default type here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, i see now.

System.currentTimeMillis(), interval.millis())
.get();

Expand Down Expand Up @@ -178,10 +177,9 @@ public void testMonitoringBulk() throws Exception {
equalTo(1L));

for (final SearchHit hit : hits.getHits()) {
assertMonitoringDoc(toMap(hit), system, "test", interval);
assertMonitoringDoc(toMap(hit), system, "monitoring_data_type", interval);
}
});
assertWarnings(RestBulkAction.TYPES_DEPRECATION_MESSAGE);
}

/**
Expand Down Expand Up @@ -264,7 +262,6 @@ private void assertMonitoringDoc(final Map<String, Object> document,

final String index = (String) document.get("_index");
assertThat(index, containsString(".monitoring-" + expectedSystem.getSystem() + "-" + TEMPLATE_VERSION + "-"));
assertThat(document.get("_type"), equalTo("doc"));
assertThat((String) document.get("_id"), not(isEmptyOrNullString()));

final Map<String, Object> source = (Map<String, Object>) document.get("_source");
Expand Down