[Actions] avoids setting a default dedupKey on PagerDuty #77773

gmmorris · 2020-09-17T14:42:51Z

Summary

The PagerDuty Action currently defaults to a dedupKey that's shared between all action executions of the same connector.
To ensure we don't group unrelated executions together this PR avoids setting a default, which means each execution will result in its own incident in PD.

As part of this change we've also made the dedupKey a required field whenever a resolve or acknowledge event_action is chosen. This ensure we don't try to resolve without a dedupKey, which would result in an error in PD.

A migration has been introduced to migrate existing alerts which might not have a dedupKey configured.

Users can set their dedupKey to ensure their executions are grouped and a follow up issue has been created to address this at a unified manner in Alerts.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-09-17T14:43:23Z

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

gmmorris · 2020-09-17T14:48:42Z

x-pack/plugins/actions/server/builtin_action_types/pagerduty.ts

+    ...omitBy(pick(params, ['timestamp', 'component', 'group', 'class']), isUndefined),
  };

-  if (params.timestamp != null) data.payload.timestamp = params.timestamp;
-  if (params.component != null) data.payload.component = params.component;
-  if (params.group != null) data.payload.group = params.group;
-  if (params.class != null) data.payload.class = params.class;
-


I made this change in the interest of making it easier to maintain longer term.
I spent a good few minutes tying to figure out why we were comparing to null a value that can only be String or undefined... I was convinced this was a bug... it took me embarassingly long to notice the != rather than !==.

Damn you Brendan Eich! 😩

To prevent others from going down the same rabbit hole, I've replaced this with an explicit check for undefined. :)

This will change the payload so that previously params that were null were not included in the payload, but now they will, with a value of null. If that's ok with the PagerDuty API, then it's fine with me :-)

gmmorris · 2020-09-18T14:48:04Z

@elasticmachine merge upstream

YulNaumenko

LGTM

gmmorris · 2020-09-21T11:23:21Z

@elasticmachine merge upstream

pmuellr

Code LGTM, but haven't tried it out yet.

For a change like this, I think we need to test a PD action against real PD, with and without a dedup key, make sure the expected things happen. Since we also changed the behavior with null payload properties (now set with a null value, instead of previously not sending the property at all), we should also test setting all of these to null to make sure PD can accept these.

If those manual tests have already been done, I'll change to approve. If not, let' do that, LMK if you need to get set up to test against real PD ...

pmuellr · 2020-09-21T12:33:31Z

x-pack/plugins/actions/server/builtin_action_types/pagerduty.ts

+    ...omitBy(pick(params, ['timestamp', 'component', 'group', 'class']), isUndefined),
  };

-  if (params.timestamp != null) data.payload.timestamp = params.timestamp;
-  if (params.component != null) data.payload.component = params.component;
-  if (params.group != null) data.payload.group = params.group;
-  if (params.class != null) data.payload.class = params.class;
-


This will change the payload so that previously params that were null were not included in the payload, but now they will, with a value of null. If that's ok with the PagerDuty API, then it's fine with me :-)

* master: (45 commits) [CSM] Use stacked chart for page views (elastic#78042) [Enterprise Search] Fix various plugin states when app has error connecting to Enterprise Search (elastic#78091) Remove service map beta badge (elastic#78039) [Enterprise Search] Rename "telemetry" to "stats" (elastic#78124) [Alerting] optimize calculation of unmuted alert instances (elastic#78021) call .destroy on ace when react component unmounts (elastic#78132) [Ingest Manager] Fix agent action acknowledgement (elastic#78089) [Upgrade Assistant] Rename "telemetry" to "stats" (elastic#78127) [Security Solution] Refactor Hosts Kpi to use Search Strategy (elastic#77606) Bump backport to 5.6.0 (elastic#78097) [Actions] adds a Test Connector tab in the Connectors list (elastic#77365) [Uptime] Improve ping chart axis (elastic#77992) [TSVB] Fields dropdowns are not populated if one of the indices is missing (elastic#77363) [UiActions] Remove duplicate apply filter action (elastic#77485) [APM] Use transaction metrics for transaction error rate (elastic#78009) [ES-ARCHIVER] Fix bug when query flag is empty (elastic#77983) Edit UI text strings in Integrations and Fleet tabs (elastic#75837) [baseline capture] switch to large workers (elastic#78109) [SECURITY_SOLUTION] list UI is backwards compatible (elastic#77956) [Mappings editor] Add support for point field type (elastic#77543) ...

* master: (31 commits) skip tests for old pacakge (elastic#78194) [Ingest Pipelines] Add url generator for ingest pipelines app (elastic#77872) [Lens] Rename "telemetry" to "stats" (elastic#78125) [CSM] Url search (elastic#77516) [Drilldowns] Config to disable URL Drilldown (elastic#77887) [Lens] Combined histogram/range aggregation for numbers (elastic#76121) Remove legacy plugins support (elastic#77599) 'Auto' interval must be correctly calculated for natural numbers (elastic#77995) [CSM] fix ingest data retry order messed up (elastic#78163) Add response status helpers (elastic#78006) Bump react-beautiful-dnd (elastic#78028) [Security Solution][Detection Engine] Bubbles up more error messages from ES queries to the UI (elastic#78004) Index pattern - refactor constructor (elastic#77791) Add `xpack.security.sameSiteCookies` to docker allow list (elastic#78192) Remove [key: string]: any; from IIndexPattern (elastic#77968) Remove requirement for manage_index_templates privilege for Index Management (elastic#77377) [Ingest Manager] Agent bulk actions UI (elastic#77690) [Metrics UI] Add inventory view timeline (elastic#77804) Reporting/Docs: Updates for setting to enable CSV Download (elastic#78101) Update to latest rum-react (elastic#78193) ...

gmmorris · 2020-09-24T11:08:19Z

@elasticmachine merge upstream

gmmorris · 2020-09-28T08:48:36Z

@elasticmachine merge upstream

kibanamachine · 2020-09-28T10:17:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 05e0ab3

History

💚 Build #76874 succeeded d1b776f
💔 Build #76840 failed e504af7
💔 Build #76514 failed 217a3c3
💔 Build #76500 failed 097b99d
💔 Build #76236 failed 56e81d2

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

pmuellr

LGTM

The PagerDuty Action currently defaults to a dedupKey that's shared between all action executions of the same connector. To ensure we don't group unrelated executions together this PR avoids setting a default, which means each execution will result in its own incident in PD. As part of this change we've also made the `dedupKey` a required field whenever a `resolve` or `acknowledge` event_action is chosen. This ensure we don't try to resolve without a dedupKey, which would result in an error in PD. A migration has been introduced to migrate existing alerts which might not have a `dedupKey` configured.

…8596) The PagerDuty Action currently defaults to a dedupKey that's shared between all action executions of the same connector. To ensure we don't group unrelated executions together this PR avoids setting a default, which means each execution will result in its own incident in PD. As part of this change we've also made the `dedupKey` a required field whenever a `resolve` or `acknowledge` event_action is chosen. This ensure we don't try to resolve without a dedupKey, which would result in an error in PD. A migration has been introduced to migrate existing alerts which might not have a `dedupKey` configured.

* master: Fix APM lodash imports (elastic#78438) Add deprecated message to tile_map and region_map visualizations. (elastic#77683) Fix Lens smokescreen flaky tests (elastic#78566) updated discover with alt text (elastic#77660) Fix types (elastic#78619) Update tutorial-visualizing.asciidoc (elastic#76977) Update tutorial-discovering.asciidoc (elastic#76976) [Search] Error notification alignment (elastic#77788) Update tutorial-define-index.asciidoc (elastic#76975) [Lens] Fieldless operations (elastic#78080) [Usage Collection] [schema] Explicit "array" definition (elastic#78141) Update tutorial-define-index.asciidoc (elastic#76973) Fix --no-basepath references in doc (elastic#78570) Move StubIndexPattern to data plugin and convert to TS. (elastic#78518) Index pattern class - remove unused methods (elastic#78538) [Security Solution] [ALL] Eliminates all console.error and console.warn from Jest output (elastic#78523) [Actions] avoids setting a default dedupKey on PagerDuty (elastic#77773) First stab at developer-focussed saved objects docs (elastic#71430) remove unnecessary config validations (elastic#78527)

avoid setting a default dedupKey

038039c

gmmorris requested a review from a team as a code owner September 17, 2020 14:42

gmmorris added Feature:Actions release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.10.0 v8.0.0 labels Sep 17, 2020

gmmorris commented Sep 17, 2020

View reviewed changes

fixed pagerduty simulator

15ea582

Merge branch 'master' into actions/pagerduty-dedup

560a885

YulNaumenko approved these changes Sep 18, 2020

View reviewed changes

Merge branch 'master' into actions/pagerduty-dedup

ce55b27

pmuellr reviewed Sep 21, 2020

View reviewed changes

gmmorris added 6 commits September 22, 2020 17:10

make dedupKey required for resolve and acknowledge

528193f

fixed pagerduty tests

06230bf

fixed typing

217a3c3

fixed test

e504af7

Merge branch 'master' into actions/pagerduty-dedup

d1b776f

Merge branch 'master' into actions/pagerduty-dedup

05e0ab3

pmuellr approved these changes Sep 28, 2020

View reviewed changes

gmmorris merged commit 8547b32 into elastic:master Sep 28, 2020

gmmorris mentioned this pull request Sep 28, 2020

[7.x] [Actions] avoids setting a default dedupKey on PagerDuty (#77773) #78596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Actions] avoids setting a default dedupKey on PagerDuty #77773

[Actions] avoids setting a default dedupKey on PagerDuty #77773

gmmorris commented Sep 17, 2020 •

edited

Loading

elasticmachine commented Sep 17, 2020

gmmorris Sep 17, 2020 •

edited

Loading

pmuellr Sep 21, 2020

gmmorris commented Sep 18, 2020

YulNaumenko left a comment

gmmorris commented Sep 21, 2020

pmuellr left a comment

pmuellr Sep 21, 2020

gmmorris commented Sep 24, 2020

gmmorris commented Sep 28, 2020

kibanamachine commented Sep 28, 2020

pmuellr left a comment

[Actions] avoids setting a default dedupKey on PagerDuty #77773

[Actions] avoids setting a default dedupKey on PagerDuty #77773

Conversation

gmmorris commented Sep 17, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Sep 17, 2020

gmmorris Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

pmuellr Sep 21, 2020

Choose a reason for hiding this comment

gmmorris commented Sep 18, 2020

YulNaumenko left a comment

Choose a reason for hiding this comment

gmmorris commented Sep 21, 2020

pmuellr left a comment

Choose a reason for hiding this comment

pmuellr Sep 21, 2020

Choose a reason for hiding this comment

gmmorris commented Sep 24, 2020

gmmorris commented Sep 28, 2020

kibanamachine commented Sep 28, 2020

💚 Build Succeeded

History

pmuellr left a comment

Choose a reason for hiding this comment

gmmorris commented Sep 17, 2020 •

edited

Loading

gmmorris Sep 17, 2020 •

edited

Loading