[alerting][actions] add task scheduled date and delay to event log #102252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pmuellr
added
Feature:Alerting
v8.0.0
release_note:skip
|
Issue #98634 mentions providing current average duration (presumably task duration, by task id (or task type?) to task executors. Did a quick review of how the task manager monitoring stats are currently handled in their HTTP route, seems like we should re-arrange some code there; some notes here: #98634 (comment) So, going to defer that for now - but I'll see if there's anything else of interest we could pull in - maybe the task id? |
pmuellr
changed the title
pmuellr
force-pushed
the
task-manager/expose-metrics-to-executors
branch
3 times, most recently
from
8585f54
to
1bb43dc
Compare
resolves elastic#98634 This adds a new object property to the event log kibana object named task, with two properties to track the time the task was scheduled to run, and the delay between when it was supposed to run and when it actually started. This task property is only added to the appropriate events. task: schema.maybe( schema.object({ scheduled: ecsDate(), schedule_delay: ecsNumber(), }) ),
pmuellr
force-pushed
the
task-manager/expose-metrics-to-executors
branch
from
1bb43dc
to
6e368f1
Compare
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
ymao1
approved these changes
Jun 21, 2021
x-pack/plugins/event_log/README.md
Outdated
| @@ -127,6 +127,10 @@ Below is a document in the expected structure, with descriptions of the fields: | |||
| // Custom fields that are not part of ECS. | |||
| kibana: { | |||
| server_uuid: "UUID of kibana server, for diagnosing multi-Kibana scenarios", | |||
| task: { | |||
| scheduled: "ISO date of when the task for this event was supposed to start", | |||
| schedule_delay: "delay in nanoseconds of when this task was supposed to start", | |||
There was a problem hiding this comment.
Suggested change
| schedule_delay: "delay in nanoseconds of when this task was supposed to start", | |
| schedule_delay: "delay in nanoseconds between when this task was supposed to start and when it actually started", |
| @@ -494,6 +497,7 @@ export class TaskRunner< | |||
|
|
|||
| const namespace = this.context.spaceIdToNamespace(spaceId); | |||
| const eventLogger = this.context.eventLogger; | |||
| const scheduleDelay = Date.now() - this.taskInstance.runAt.getTime(); | |||
There was a problem hiding this comment.
nit: Maybe use runDate so that the logged start time will match exactly with the calculated schedule delay?
|
@elasticmachine merge upstream |
mikecote
approved these changes
Jun 22, 2021
Comment on lines
+153
to
+156
| task: { | ||
| scheduled: taskInfo.scheduled.toISOString(), | ||
| schedule_delay: Millis2Nanos * (Date.now() - taskInfo.scheduled.getTime()), | ||
| }, |
There was a problem hiding this comment.
Based on your comment here: #102252 (comment). Task id could be interesting as well, or it could also belong in the event log's kibana saved objects array to complete the dependency train.
There was a problem hiding this comment.
heh, ya, didn't get around to it, but we do have the top-level task object we can provide more goodies for later.
One of the issues with adding the task manager SO reference, is that we'd also have to indicate the index, since it's not in .kibana - or I guess that's my understanding, that you'd need to know the index name to actually look them up. Or we could special case some of the "types" I suppose, if the existing SO client doesn't magically find them.
There was a problem hiding this comment.
One of the issues with adding the task manager SO reference, is that we'd also have to indicate the index
I think this would be ok as long as the task SO is accessed via the saved objects client/repository. The SO repository would automatically know it's a type of task and look up the index task belongs into. All good to defer this 👍
pmuellr
added a commit
to pmuellr/kibana
that referenced
this pull request
Jun 23, 2021
…lastic#102252) resolves elastic#98634 This adds a new object property to the event log kibana object named task, with two properties to track the time the task was scheduled to run, and the delay between when it was supposed to run and when it actually started. This task property is only added to the appropriate events. task: schema.maybe( schema.object({ scheduled: ecsDate(), schedule_delay: ecsNumber(), }) ),
|
This was reverted with 3e952fa due to a type failure - https://kibana-ci.elastic.co/job/elastic+kibana+master/15004/execution/node/413/log/ |
ymao1
pushed a commit
to ymao1/kibana
that referenced
this pull request
Jun 23, 2021
…lastic#102252) resolves elastic#98634 This adds a new object property to the event log kibana object named task, with two properties to track the time the task was scheduled to run, and the delay between when it was supposed to run and when it actually started. This task property is only added to the appropriate events. task: schema.maybe( schema.object({ scheduled: ecsDate(), schedule_delay: ecsNumber(), }) ),
9 tasks
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/jobs/close_jobs·ts.apis Machine Learning jobs close_jobs close jobs fail because they are running as ML PoweruserStandard OutStack TraceKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/jobs/close_jobs·ts.apis Machine Learning jobs close_jobs close jobs fail because they are running as ML PoweruserStandard OutStack TraceMetrics [docs]
History
To update your PR or re-run it, just comment with: |
pmuellr
added a commit
that referenced
this pull request
Jun 24, 2021
#103172) resolves #98634 This adds a new object property to the event log kibana object named task, with two properties to track the time the task was scheduled to run, and the delay between when it was supposed to run and when it actually started. This task property is only added to the appropriate events. task: schema.maybe( schema.object({ scheduled: ecsDate(), schedule_delay: ecsNumber(), }) ), Note that these changes were previously merged to master in #102252 which had to be reverted - this PR contains the same commits, plus some additional ones to resolve the tests that were broken during the bad merge.
pmuellr
added a commit
to pmuellr/kibana
that referenced
this pull request
Jun 24, 2021
elastic#103172) resolves elastic#98634 This adds a new object property to the event log kibana object named task, with two properties to track the time the task was scheduled to run, and the delay between when it was supposed to run and when it actually started. This task property is only added to the appropriate events. task: schema.maybe( schema.object({ scheduled: ecsDate(), schedule_delay: ecsNumber(), }) ), Note that these changes were previously merged to master in elastic#102252 which had to be reverted - this PR contains the same commits, plus some additional ones to resolve the tests that were broken during the bad merge.
pmuellr
added a commit
that referenced
this pull request
Jun 24, 2021
#103172) (#103296) resolves #98634 This adds a new object property to the event log kibana object named task, with two properties to track the time the task was scheduled to run, and the delay between when it was supposed to run and when it actually started. This task property is only added to the appropriate events. task: schema.maybe( schema.object({ scheduled: ecsDate(), schedule_delay: ecsNumber(), }) ), Note that these changes were previously merged to master in #102252 which had to be reverted - this PR contains the same commits, plus some additional ones to resolve the tests that were broken during the bad merge.
This was referenced Jun 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: this PR was reverted after being merged: #102252 (comment)
A new PR #103172 was created to merge this code
resolves #98634
Summary
This adds a new object property to the event log
kibanaobject namedtask, with two properties to track the time the task was scheduled to run, and the delay between when it was supposed to run and when it actually started. Thistaskproperty is only added to theprovider: alerting; action: executeandprovider:actions; action: executeevents.I also happened to notice that a
namespaceproperty got added to theruleobject in the event log documents, coming from this PR: #101132 . Problem is, that field does not exist in ECS! At least in the versions I looked at: https://github.com/elastic/ecs/blob/master/schemas/rule.yml . I'm curious how this happened - but here's a guess: when generating the event log schema, we depend on having the ecs repo checked out next to Kibana, and then we pull data from that. We actually check (somehow) that you have a tagged version checked out, but we assume the code hasn't been changed since the checkout. If it was changed for some reason - to do some testing, experimenting, etc - we'd use that version. We probably need to add an additional safety check, somehow. Like read one of the other "list of fields" files available in the ecs repo, as a double-check that the field is an ECS field.Here's what the
rulelooks like in the event log, before this PR:kibana/x-pack/plugins/event_log/generated/mappings.json
Lines 173 to 222 in dc5972c
I went ahead and removed all the references to the
namespacefield.Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportAny UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listThis renders correctly on smaller devices using a responsive layout. (You can test this in your browser)This was checked for cross-browser compatibilityFor maintainers
This was checked for breaking API changes and was labeled appropriately