-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ML-DataFrame] add a stats endpoint #35911
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-DataFrame] add a stats endpoint #35911
Conversation
|
Pinging @elastic/ml-core |
772b9fd to
f2c427d
Compare
f2c427d to
4754f0d
Compare
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.
It seems all 3 fields should not be null. Add Objects.requireNotNull checks? (Btw, just realised the main reason I like those is the way they serve as documentation)
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.
Also init jobsStateAndStats to empty list?
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.
You should be able to simply do builder.field(JOBS.getPreferredName(), jobsStateAndStats) here. Unless we need to use Params it should be fine.
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.
This might be simpler if instead of declaring the return value we call the listener once for each branch directly like:
listener.onResponse(CollectionsSingletonList(jobStateAndStats)
etc. Just a suggestion, I know this is quite subjective :-)
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 think you should be able to squash this comment in 2 lines
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.
Eclipse formatting drives me crazy! -> #35984
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.
For anomaly detection, the equivalent action is called GetJobStatsAction where job is singular. We might want to be consistent but I don't feel strongly about it.
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.
Discussed this with @droberts195 on a previous PR and we went all for plural, all the endpoint have changed to .../jobs/{id}/... GetDataFrameJob changed to GetDataFrameJobs, etc.
Anyway, it's a good point and I suggest to keep it as is for now but have a session about naming where we can go over all endpoints and then do a renaming PR.
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 ML actions are inconsistent in this respect: we have GetJobsStatsAction and GetDatafeedsStatsAction but GetJobStatsActionRequest and RestGetJobStatsAction. I think plural is correct.
|
addressed the review comments. As #35471 has been merged yesterday, the stats API is going to break with the next merge, I will therefore do a master merge now and add fixes as part of this PR. |
bce1ef4 to
c9137dd
Compare
|
@droberts195 @dimitris-athanasiou Thanks for your comments, this PR should be ready now. |
droberts195
left a comment
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
dimitris-athanasiou
left a comment
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
|
Oh, just realised this is missing the REST endpoint specification which also enables writing YAML tests. |
|
@dimitris-athanasiou Yes, you are right. This is true for all endpoints, tracked in the team repro issue number 7. |
Feature Branch PR
add a stats endpoint for data frame jobs:
(instead of
_allyou can specify an exact/explicit job id)example output:
Notes:
_xpack/feature...) will be renamed in a separate PR.