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] Create the ML annotations index #36731

Merged
merged 2 commits into from Dec 18, 2018

Conversation

droberts195
Copy link
Contributor

The ML UI now provides the ability for users to annotate
time periods with arbitrary text to add insight to what
happened.

This change makes the backend create the index for these
annotations, together with read and write aliases to
make future upgrades possible without adding complexity
to the UI.

It also adds read and write permission to the index for
all ML users (not just admins).

The spec for the index is in
https://github.com/elastic/kibana/pull/26034/files#diff-c5c6ac3dbb0e7c91b6d127aa06121b2cR7

Relates #33376
Relates elastic/kibana#26034

The ML UI now provides the ability for users to annotate
time periods with arbitrary text to add insight to what
happened.

This change makes the backend create the index for these
annotations, together with read and write aliases to
make future upgrades possible without adding complexity
to the UI.

It also adds read and write permission to the index for
all ML users (not just admins).

The spec for the index is in
https://github.com/elastic/kibana/pull/26034/files#diff-c5c6ac3dbb0e7c91b6d127aa06121b2cR7

Relates elastic#33376
Relates elastic/kibana#26034
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@davidkyle
Copy link
Member

run gradle build tests 1

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

Left a few minor comments

public static final ParseField ANNOTATION = new ParseField("annotation");
public static final ParseField CREATE_TIME = new ParseField("create_time");
public static final ParseField CREATE_USERNAME = new ParseField("create_username");
public static final ParseField TIMESTAMP = new ParseField("timestamp");
Copy link
Member

Choose a reason for hiding this comment

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

is timestamp the partner of end_timestamp? Should it be start_timestamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason timestamp was chosen is that originally we'd intended annotations to be stored in the results index. Then we decided to switch to a separate index, but didn't change the field names.

I think it's too late to change now as it's feature freeze day and
https://github.com/elastic/kibana/pull/26034/files#diff-c5c6ac3dbb0e7c91b6d127aa06121b2cR7 is already merged.

But one thing I just noticed is that end_timestamp is optional in the UI code, so actually that makes timestamp seem not as bad, as it could either be the start or the sole time. But I need to update this PR to account for end_timestamp being optional.

new Date(randomNonNegativeLong()),
new Date(randomNonNegativeLong()),
randomBoolean() ? randomAlphaOfLengthBetween(10, 30) : null,
new Date(randomNonNegativeLong()),
Copy link
Member

Choose a reason for hiding this comment

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

This field and the next one can be null perhaps use randomBoolean() ? as above

}

@Before
public void waitForMlTemplates() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

This method is defined in MlSingleNodeTestCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only on the jindex branches. I'll add a TODO to tidy up when both are merged.

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

@droberts195 droberts195 merged commit 6243074 into elastic:master Dec 18, 2018
@droberts195 droberts195 deleted the create_annotations_index branch December 18, 2018 12:18
droberts195 added a commit that referenced this pull request Dec 18, 2018
The ML UI now provides the ability for users to annotate
time periods with arbitrary text to add insight to what
happened.

This change makes the backend create the index for these
annotations, together with read and write aliases to
make future upgrades possible without adding complexity
to the UI.

It also adds read and write permission to the index for
all ML users (not just admins).

The spec for the index is in
https://github.com/elastic/kibana/pull/26034/files#diff-c5c6ac3dbb0e7c91b6d127aa06121b2cR7

Relates #33376
Relates elastic/kibana#26034
client.admin().indices()::aliases);
}, finalListener::onFailure);

// Only create the index or aliases if some other ML index exists - saves clutter if ML is never used.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the method is called createAnnotationsIndex, it would be nice to reflect this, e.g. maybeCreate... or move the "maybe"-part out of createAnnotationsIndex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is already merged, but I'll add maybe if there's a followup PR for any reason

droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Dec 19, 2018
Fixes two minor problems reported after merge of elastic#36731:

1. Name the creation method to make clear it only creates
   if necessary
2. Avoid multiple simultaneous in-flight creation requests
droberts195 added a commit that referenced this pull request Dec 19, 2018
Fixes two minor problems reported after merge of #36731:

1. Name the creation method to make clear it only creates
   if necessary
2. Avoid multiple simultaneous in-flight creation requests
ywelsch pushed a commit that referenced this pull request Dec 19, 2018
Fixes two minor problems reported after merge of #36731:

1. Name the creation method to make clear it only creates
   if necessary
2. Avoid multiple simultaneous in-flight creation requests
ywelsch pushed a commit that referenced this pull request Dec 19, 2018
Fixes two minor problems reported after merge of #36731:

1. Name the creation method to make clear it only creates
   if necessary
2. Avoid multiple simultaneous in-flight creation requests
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.

None yet

5 participants