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.

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

[ML] Pass extra JSON config files to autodetect #66736

Merged

Conversation

edsavage
Copy link
Contributor

Pass JSON files containing rule filters and scheduled
events configuration to autodetect.

This change must not be merged until after elastic/ml-cpp#1640 as the
C++ backend code must be able to safely consume the new command line
options.

Relates elastic/ml-cpp#1253

Pass JSON files containing rule filters and scheduled
events configuration to autodetect.

This change must not be merged until after elastic/ml-cpp#1640 as the
C++ backend code must be able to safely consume the new command line
options.

Relates elastic/ml-cpp#1253
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

Comment on lines 62 to 77
public void writeJson() throws IOException {
buffer.append('[');
boolean first = true;
for (ScheduledEvent event : scheduledEvents) {
if (first) {
first = false;
} else {
buffer.append(',');
}

try (XContentBuilder contentBuilder = XContentFactory.jsonBuilder()) {
buffer.append(Strings.toString(event.writeJson(bucketSpan, contentBuilder)));
}
}
buffer.append(']');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's very non-standard in Elasticsearch to have a JSON document whose outermost level is an array. That's why you've had to hand-craft a writer instead of using standard mechanisms. I think instead you should use a format similar to the get scheduled events API response, with the document being an object at its outermost level, and containing a field events that is the array of scheduled events. If you can tolerate the superfluous count you could actually reuse GetCalendarEventsAction.Response to create and print this. Or create a new very simple ToXContentObject that contains a List<ScheduledEvent> and uses builder.field("events", scheduledEvents) in its toXContent method.

Then you'll need to change the C++ parser to accept the extra object layer and events field, but there needs to be a second PR on the C++ side anyway and since the current C++ code is disabled this PR can still be merged before the second round of C++ updates.

@edsavage
Copy link
Contributor Author

jenkins run elasticsearch-ci/2

Modified JSON configuration format for both scheduled events and for
filters to be objects (rather than arrays).
Path eventsConfigFile = Files.createTempFile(env.tmpFile(), "eventsConfig", JSON_EXTENSION);
filesToDelete.add(eventsConfigFile);

List<DetectionRuleWriter> detectionRuleWriters = scheduledEvents.stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

As we're streaming scheduled events, I think it would be clearer to map these to a ScheduledEventToRuleWriter. Then the RULES ParseField added in ScheduledEvent can move in that writer class.

Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

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

LGTM

@edsavage edsavage merged commit ffc1b3c into elastic:master Jan 4, 2021
edsavage added a commit that referenced this pull request Jan 5, 2021
Pass JSON files containing rule filters and scheduled
events configuration to autodetect.

This change must not be merged until after elastic/ml-cpp#1640 as the
C++ backend code must be able to safely consume the new command line
options.

Relates elastic/ml-cpp#1253
Backports #66736
@edsavage edsavage deleted the anomaly_job_config_json_filters_and_events branch January 25, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants