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

[Alerting & Actions ] More debug logging #85149

Merged
merged 11 commits into from
Dec 8, 2020

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Dec 7, 2020

Resolves #49401

Redo of this PR #84670 because I mucked that up with a bad merge from master.

Summary

Added debug logging for:

  • Log start of each alert execution
  • If active instances exist for alert execution, log how many and the instance ids and action group of each
  • If recovered instances exist for alert execution, log how many and the instance ids of each
  • Log if action execution for alert is skipped due to muteAll
  • Log if action execution for individual alert instance is skipped due to mute or throttle
  • Log start of each action execution

Verified that error logging for alert and action execution errors log any detailed messages that bubble up


generateNewAndRecoveredInstanceEvents({
eventLogger,
originalAlertInstances,
currentAlertInstances: instancesWithScheduledActions,
recoveredAlertInstances,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed based on suggestion from previous PR: #84670 (comment)

@@ -333,12 +356,15 @@ export class TaskRunner {
schedule: taskSchedule,
} = this.taskInstance;

const runDate = new Date().toISOString();
this.logger.debug(`executing alert ${this.alertType.id}:${alertId} at ${runDate}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question from previous PR:

Was the runDate added to match the Kibana logs with the event log data timestamp when debugging?

Yes, that was my intention

@@ -120,6 +120,8 @@ export class ActionExecutor {
}

const actionLabel = `${actionTypeId}:${actionId}: ${name}`;
logger.debug(`executing action ${actionLabel}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From previous PR:

With the logger prefixing all the logs with a timestamp, is there a reason for the extra at ${new Date().toISOString()} here?

Updated to remove timestamp from debug message

@ymao1
Copy link
Contributor Author

ymao1 commented Dec 7, 2020

I was also wondering if we should log (as debug) the raw errors the connectors receive when they encounter an error. I know some of them will do some processing on the error to ensure we don't return sensitive information (ex: credentials) but maybe using debug is ok? I'm not sure..

Discussed this comment #84670 (comment) from the previous PR offline and decided that this will not be included in this PR. @mikecote is there another issue for this already? should i make an issue?

@ymao1 ymao1 self-assigned this Dec 7, 2020
@ymao1 ymao1 added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0 labels Dec 7, 2020
@ymao1 ymao1 marked this pull request as ready for review December 7, 2020 15:53
@ymao1 ymao1 requested a review from a team as a code owner December 7, 2020 15:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@ymao1
Copy link
Contributor Author

ymao1 commented Dec 7, 2020

@mikecote we had discussed adding logging around the loadAlertAttributesAndRun as part of the investigation into #84335 but that has already been done as part of this PR

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Few nits on the message to indicate scheduling instead of executing.

x-pack/plugins/alerts/server/task_runner/task_runner.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerts/server/task_runner/task_runner.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerts/server/task_runner/task_runner.ts Outdated Show resolved Hide resolved
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM w/Mike's suggestions

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 46960 47720 +760

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ymao1 ymao1 merged commit 18a99fc into elastic:master Dec 8, 2020
ymao1 added a commit to ymao1/kibana that referenced this pull request Dec 8, 2020
* Adding debug messages

* Adding timestamp to action execution log

* Jest tests

* Merging in master

* PR fixes

* Cleanup

* PR fixes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ymao1 added a commit that referenced this pull request Dec 9, 2020
* Adding debug messages

* Adding timestamp to action execution log

* Jest tests

* Merging in master

* PR fixes

* Cleanup

* PR fixes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 9, 2020
…k-field-to-hot-phase

* 'master' of github.com:elastic/kibana: (429 commits)
  simplify popover open state logic (elastic#85379)
  [Logs UI][Metrics UI] Move actions to the kibana header (elastic#84648)
  [Search Source] Do not pick scripted fields if * provided (elastic#85133)
  [Search] Session SO polling (elastic#84225)
  [Transform] Replace legacy elasticsearch client (elastic#84932)
  [Uptime]Refactor header and action menu (elastic#83779)
  Fix agg select external link (elastic#85380)
  [ILM] Show forcemerge in hot when rollover is searchable snapshot is enabled (elastic#85292)
  clear using keyboard (elastic#85042)
  [GS] add tag and dashboard suggestion results (elastic#85144)
  [ML] API integration tests - skip GetAnomaliesTableData
  Add ECS field for event.code. (elastic#85109)
  [Functional][TSVB] Wait for markdown textarea to be cleaned (elastic#85128)
  skip flaky suite (elastic#62060)
  skip flaky suite (elastic#85098)
  Bump highlight.js to v9.18.5 (elastic#84296)
  Add `server.publicBaseUrl` config (elastic#85075)
  [Alerting & Actions ] More debug logging (elastic#85149)
  [Security Solution][Case] Manual attach alert to a case (elastic#82996)
  Loosen UUID regex to accept uuidv1 or uuidv4 (elastic#85338)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/index.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts
#	x-pack/plugins/index_lifecycle_management/server/routes/api/policies/register_create_route.ts
@ymao1 ymao1 deleted the alerting/debug-logging branch February 4, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add alert and action plugin console logging (ease of debugging)
5 participants