-
Notifications
You must be signed in to change notification settings - Fork 25.5k
[ML] Correct small inconsistencies in ml APIs spec and docs #39801
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
Conversation
Pinging @elastic/ml-core |
(string) The timestamp for the end of the scheduled event. The datetime string | ||
is in ISO 8601 format. | ||
(string) The timestamp for the end of the scheduled event | ||
in milliseconds since the epoch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we're doing people a disservice by explicitly saying it's only milliseconds since the epoch. We accept times in the default format accepted by Elasticsearch, which is strict_date_optional_time||epoch_millis
. This default is widely used but only documented in a half-arsed way, in https://www.elastic.co/guide/en/elasticsearch/reference/current/date.html.
I just looked at our other endpoint documentation and it often doesn't specify the format for times supplied as URL arguments. One that does is start datafeeds, and that uses a bulleted list separate to the parameter descriptions, but it does say that both epoch and ISO 8601 times are supported.
Anyway, I'd prefer that if we specify anything we make it clear that we're flexible, so I would change in milliseconds since the epoch
to in milliseconds since the epoch or ISO 8601 format
in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scheduled Events can be created with timestamps in either ISO 8601 or epoch millis, any view of the object will have the timestamps in epoch millis. I pushed a change for the POST _ml/calendars/<calendar_id>/events
docs explaining the format options similar to the example you gave where it is the format of the API params that are documented rather than the resource fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now there's a discrepancy between the two files, as the POST _ml/calendars/<calendar_id>/events
docs say the start and end time can be a string with epoch millis or ISO 8601, and then there's a cross reference to this file where the detailed docs say the type is long
, which rules out ISO 8601. I think this file should also say both formats are possible and list the type as string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other ml resource objects use the date
type for timestamps e.g. buckets https://www.elastic.co/guide/en/elasticsearch/reference/6.2/ml-results-resource.html#ml-results-buckets I changed the event resource docs to be the same.
The event can be created with a string but thereafter it will always be a long I think saying it is a string is just plain wrong as any API request will return the value a long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, true, string
would be wrong. date
is good, because presumably it reacts to ?human
, so date
makes that clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
human
is tricky not least because it is scarcely mentioned in the docs but it also creates a new field suffixed with _string
i.e. start_time_string
is the human readable field. I guess it would break clients otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wasn't suggesting we mention ?human
in this doc, just that saying the field type is date
makes it clearer that ?human
will work for human clients. I would never expect the language clients to use ?human
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #39677
body
was null which makes the language clients think that is not the case.Closes #39673 by registering the missing URL for delete forecast where the forecast id is not specified.
Closes #39676 with a doc change in eventresource.asciidoc.