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

[Transform] report last search time in transform stats #66718

Merged
merged 7 commits into from
Jan 20, 2021

Conversation

hendrikmuhs
Copy link

@hendrikmuhs hendrikmuhs commented Dec 21, 2020

transforms reports the the last time changes where detected with changes_last_detected_at, however that doesn't tell a user it searched for changes, this PR adds a field last_search_time to report when transform searched for changes the last time.

fixes #66410
relates #66367

Notes

Review

the main changes are part of bd59629 (#66718), the rest are style and test fixes

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml/Transform)

@@ -355,6 +357,8 @@ protected void onStart(long now, ActionListener<Boolean> listener) {
}));
} else {
hasSourceChanged = true;
context.setChangesLastSearchedAt(instantOfTrigger);
context.setChangesLastDetectedAt(instantOfTrigger);
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes behaviour for changed_last_detected_at. The change seems correct but is it fixing another issue with that not getting updated properly?

Copy link
Author

Choose a reason for hiding this comment

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

yes, the if is "continuous" mode, the else is "batch". I see no reason why we report changed_last_detected_at (and changed_last_searched_at) only for continuous but not for batch. This makes it more consistent.

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

@hendrikmuhs hendrikmuhs force-pushed the transform-add-last-search branch from b1bd39a to 7d9510f Compare January 11, 2021 09:44
@hendrikmuhs hendrikmuhs changed the title [Transform] add last_searched_at field [Transform] add changes_last_searched_at field Jan 13, 2021
@hendrikmuhs hendrikmuhs changed the title [Transform] add changes_last_searched_at field [Transform] report last time transform searched for changes on stats Jan 19, 2021
@hendrikmuhs hendrikmuhs force-pushed the transform-add-last-search branch from 7d9510f to db46fe0 Compare January 19, 2021 10:45
Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

@hendrikmuhs hendrikmuhs force-pushed the transform-add-last-search branch from b456f23 to 17854c8 Compare January 20, 2021 10:42
@hendrikmuhs hendrikmuhs changed the title [Transform] report last time transform searched for changes on stats [Transform] report last search time in transform stats Jan 20, 2021
@hendrikmuhs hendrikmuhs merged commit 951d822 into elastic:master Jan 20, 2021
@hendrikmuhs hendrikmuhs deleted the transform-add-last-search branch January 20, 2021 14:00
hendrikmuhs pushed a commit that referenced this pull request Jan 21, 2021
…67779)

transforms reports the last time changes where detected with changes_last_detected_at, however
that doesn't tell a user it searched for changes, this PR adds a field last_search_time to report
when transform searched for changes the last time.

fixes #66410
relates #66367
backport #66718
hendrikmuhs pushed a commit that referenced this pull request Jan 21, 2021
adjusts the version after the backport of #66718 and re-enables BWC
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.

[CI] TransformContinuousIT.testContinousEvents failing
5 participants