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

[APM] Threshold alerts #59566

Merged
merged 7 commits into from Mar 24, 2020
Merged

[APM] Threshold alerts #59566

merged 7 commits into from Mar 24, 2020

Conversation

@dgieselaar
Copy link
Contributor

dgieselaar commented Mar 6, 2020

Closes #49038 (actually, does it? should we remove the watcher functionality in this PR as well we'll keep the Watcher one as well for the time being).

Out of scope:

  • header redesign (open issue #40236). Added a menu next to the integrations dropdown for now.
  • annotations in charts (needs the event log to be queryable) #60373
  • dropdown in chart (could be added, LMK if important enough)

I've not yet tested all scenarios, but it would be helpful to get some feedback while I'm out on spacetime so I can pick it up directly after.

Related issues:

  • Filtering the alert list on APM alerts: #59545
  • Error handling in the alert form: #58181
  • System tags: #58417
  • Event log: #55633

Relevant changes for @elastic/kibana-alerting-services are mostly in a77f088 and 73d6bdc.

image
image
image
image

@dgieselaar dgieselaar requested review from elastic/apm-ui as code owners Mar 6, 2020
@dgieselaar dgieselaar force-pushed the dgieselaar:error-group-alert branch from a77f088 to 564dbc9 Mar 9, 2020
'save',
'alerting:show',
'actions:show',
'alerting:save',

This comment has been minimized.

Copy link
@sqren

sqren Mar 10, 2020

Member

Is it possible to grant users granular access, like alerting:show bu not actions:show?

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Mar 16, 2020

Author Contributor

Possible for us? Or for admins?

@dgieselaar

This comment has been minimized.

Copy link
Contributor Author

dgieselaar commented Mar 10, 2020

@@ -88,22 +88,31 @@ export const apm: LegacyPluginInitializer = kibana => {
catalogue: ['apm'],
privileges: {
all: {
api: ['apm', 'apm_write'],
api: ['apm', 'apm_write', 'actions-read', 'alerting-read'],

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

apm_write uses snake_case. Should we stay consistent?

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

Btw. Should apm be renamed to apm_read?

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

ah.. actions-read is what's defined in the Action app endpoints...
My mind is a little hazy on this mapping concept. Are we mapping tags to privileges here? How does it work?

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

I understood it last time I looked into it, but that was last year. It seems this mapping concept is not immediately intuitive so adding some comments (albeit a bit verbose) might be a good idea.

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Mar 16, 2020

Author Contributor

Would just linking to x-pack/plugins/features/common/feature_kibana_privileges.ts where it's explained be OK? Rather than copying documentation between files?

AlertAdd
} from '../../../../../../../../../plugins/triggers_actions_ui/public';

type AlertAddProps = React.ComponentProps<typeof AlertAdd>;

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

This is such an underrated ts helper

parsedInterval = parseEsInterval(duration);
} catch (err) {
//
}

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

Should the error be handled somehow?

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Mar 16, 2020

Author Contributor

It shouldn't, but I'll leave a comment there with the reason.

This comment has been minimized.

Copy link
@sqren

sqren Mar 16, 2020

Member

In that case this feels like something that shouldn't be thrown from parseEsInterval. But I can see how that is not easily changed.

value={parsedInterval?.value ?? ''}
onChange={e => onChange(`${e.target.value}${displayedUnit.current}`)}
prepend={i18n.translate(
'xpack.apm.serviceAlertTrigger.durationField..last',

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

This key looks off durationField..last

//
}

const displayedUnit = useRef(parsedInterval?.unit);

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

Why is the ref needed?

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Mar 16, 2020

Author Contributor

To allow the user to clear the number input. I'll add a comment there as well:

  // we use a ref that we only update when we've succesfully parsed
  // the interval. this allows the user to clear the number input
  // without us having to fill it with a 0 in order for it to be
  // succesfully parsed.

This comment has been minimized.

Copy link
@sqren

sqren Mar 16, 2020

Member

Why do we have to fill it with a 0? Is "" (empty string) not valid?

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Mar 20, 2020

Author Contributor

That's what this is (was) about - supporting anything that is not a valid number, which is e.g. what happens when the user clears the field. Once that happens we can no longer successfully parse the interval.

In any case I'm now using the ForLastExpression that the triggers_actions_ui plugin provides.

});

// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

This comment has been minimized.

Copy link
@sqren

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Mar 16, 2020

Author Contributor

😬I'll add a comment.

@dgieselaar dgieselaar force-pushed the dgieselaar:error-group-alert branch from 564dbc9 to c0dc3c7 Mar 12, 2020
@sqren
sqren approved these changes Mar 12, 2020
`service.name:${params.serviceName}`,
`transaction.type:${params.transactionType}`
Comment on lines 72 to 73

This comment has been minimized.

Copy link
@sqren

sqren Mar 12, 2020

Member

I think we should use the ecs constants:

Suggested change
`service.name:${params.serviceName}`,
`transaction.type:${params.transactionType}`
`${SERVICE_NAME}:${params.serviceName}`,
`${TRANSACTION_TYPE}:${params.transactionType}`

Ntw. what do you think about renaming elasticsearch_fieldnames.ts to ecs.ts ?

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Mar 16, 2020

Author Contributor

It's not all ECS (unfortunately). ie, service.environment is not in the ECS specification. Same goes for error.grouping_key and a couple of others. So maybe elasticsearch_fieldnames is good enough for now?

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Mar 16, 2020

Author Contributor

btw, I'm removing the service.name and transaction.type tags. We're not doing anything wiht lookups now, and in the future (IIRC) we can use the params.

@formgeist

This comment has been minimized.

Copy link
Contributor

formgeist commented Mar 12, 2020

@dgieselaar I've collected some immediate design feedback and polish comments;

  1. Can we add some separation between these two form fields? 8px like the others?

Screenshot 2020-03-12 at 14 40 34

Screenshot 2020-03-12 at 14 40 43

  1. I suppose we need the transaction.type selection and tag is needed to indicate the type. Wonder if we need to make this part of the trigger definition and prefill with the selected type from the Transaction page. Secondly, is the transaction.type needed for error rate alerts?

Screenshot 2020-03-12 at 14 40 53

transaction.type tag shows for Error rate alerts.

Screenshot 2020-03-12 at 15 08 35

  1. Do we control the copy in these tooltips?

Screenshot 2020-03-12 at 14 41 08

  1. Add a spacer between the trigger definitions and the alert actions. Probably a EuiSpacer size: m.

Screenshot 2020-03-12 at 14 56 32

  1. Noticed that the Transaction duration alert has the type Error rate.

Screenshot 2020-03-12 at 14 57 58

  1. Another spacing form fields in the Error rate flyout

Screenshot 2020-03-12 at 15 23 35

Also, can you tell me how exactly we calculate the Error rate? The number of new errors over the selected time range? Not sure the label title and append copy are good enough...

Looks great otherwise 🙌

@peterschretlen peterschretlen mentioned this pull request Mar 13, 2020
1 of 3 tasks complete
Copy link
Contributor

YulNaumenko left a comment

LGTM. Happy to see one more alerting integration!

Copy link
Contributor

formgeist left a comment

Some minor label changes that move for consistency across Observability apps.

name: i18n.translate(
'xpack.apm.serviceDetails.alertsMenu.viewActiveAlerts',
{
defaultMessage: 'View active alerts'

This comment has been minimized.

Copy link
@formgeist

formgeist Mar 18, 2020

Contributor
Suggested change
defaultMessage: 'View active alerts'
defaultMessage: 'Manage alerts'
const createThresholdAlertLabel = i18n.translate(
'xpack.apm.serviceDetails.alertsMenu.createThresholdAlert',
{
defaultMessage: 'Create threshold alert'

This comment has been minimized.

Copy link
@formgeist

formgeist Mar 18, 2020

Contributor
Suggested change
defaultMessage: 'Create threshold alert'
defaultMessage: 'Create alert'
name: i18n.translate(
'xpack.apm.serviceDetails.alertsMenu.transactionDuration',
{
defaultMessage: 'Transaction duration'

This comment has been minimized.

Copy link
@formgeist

formgeist Mar 18, 2020

Contributor
Suggested change
defaultMessage: 'Transaction duration'
defaultMessage: 'Transaction duration threshold'
name: i18n.translate(
'xpack.apm.serviceDetails.alertsMenu.errorRate',
{
defaultMessage: 'Error rate'

This comment has been minimized.

Copy link
@formgeist

formgeist Mar 18, 2020

Contributor
Suggested change
defaultMessage: 'Error rate'
defaultMessage: 'Error rate threshold'
@dgieselaar dgieselaar force-pushed the dgieselaar:error-group-alert branch from 5a28653 to fcb78c7 Mar 19, 2020
@dgieselaar dgieselaar requested a review from elastic/kibana-app-arch as a code owner Mar 19, 2020
@dgieselaar dgieselaar force-pushed the dgieselaar:error-group-alert branch from fcb78c7 to cdf77ce Mar 23, 2020
@dgieselaar dgieselaar removed the request for review from elastic/kibana-app-arch Mar 23, 2020
@dgieselaar dgieselaar force-pushed the dgieselaar:error-group-alert branch 2 times, most recently from 4a9bcf5 to bd7a6f0 Mar 23, 2020
@dgieselaar dgieselaar force-pushed the dgieselaar:error-group-alert branch from bd7a6f0 to 5ff8d75 Mar 24, 2020
@kibanamachine

This comment has been minimized.

Copy link

kibanamachine commented Mar 24, 2020

💚 Build Succeeded

History

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

@dgieselaar dgieselaar merged commit 85c0be3 into elastic:master Mar 24, 2020
55 checks passed
55 checks passed
API integration tests node scripts/functional_tests --config test/api_integration/config.js --bail --debug
Details
Browser tests yarn run grunt test:karma-ci
Details
Build kbn_tp_sample_panel_action yarn build
Details
CLA All commits passed the check
Details
Check core API changes node scripts/check_published_api_changes
Details
Check file casing node scripts/check_file_casing --quiet
Details
Check licenses node scripts/check_licenses --dev
Details
Check lockfile symlinks node scripts/check_lockfile_symlinks --quiet
Details
Example functional tests node scripts/functional_tests --config test/examples/config.js --bail --debug
Details
Functional tests / Group 1 yarn run grunt run:functionalTests_ciGroup1
Details
Functional tests / Group 10 yarn run grunt run:functionalTests_ciGroup10
Details
Functional tests / Group 11 yarn run grunt run:functionalTests_ciGroup11
Details
Functional tests / Group 12 yarn run grunt run:functionalTests_ciGroup12
Details
Functional tests / Group 2 yarn run grunt run:functionalTests_ciGroup2
Details
Functional tests / Group 3 yarn run grunt run:functionalTests_ciGroup3
Details
Functional tests / Group 4 yarn run grunt run:functionalTests_ciGroup4
Details
Functional tests / Group 5 yarn run grunt run:functionalTests_ciGroup5
Details
Functional tests / Group 6 yarn run grunt run:functionalTests_ciGroup6
Details
Functional tests / Group 7 yarn run grunt run:functionalTests_ciGroup7
Details
Functional tests / Group 8 yarn run grunt run:functionalTests_ciGroup8
Details
Functional tests / Group 9 yarn run grunt run:functionalTests_ciGroup9
Details
Internationalization check node scripts/i18n_check --ignore-missing
Details
Interpreter functional tests node scripts/functional_tests --config test/interpreter_functional/config.ts --bail --debug --kibana-install-dir /dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64-1
Details
Jest integration tests yarn run grunt test:jest_integration
Details
Jest tests yarn run grunt test:jest
Details
Kibana accessibility tests node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64 --config test/accessibility/config.ts
Details
Mocha tests node scripts/mocha
Details
Node.js hardening tests node scripts/test_hardening.js
Details
Plugin functional tests node scripts/functional_tests --config test/plugin_functional/config.js --bail --debug
Details
Project tests yarn run grunt test:projects
Details
Type check node scripts/type_check
Details
TypeScript - all files belong to a TypeScript project node scripts/check_ts_projects
Details
Verify NOTICE.txt node scripts/notice --validate
Details
Verify dependency versions yarn run grunt verifyDependencyVersions
Details
X-Pack Chrome Functional tests / Group 1 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-1 --include-tag ciGroup1
Details
X-Pack Chrome Functional tests / Group 10 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-10 --include-tag ciGroup10
Details
X-Pack Chrome Functional tests / Group 2 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-2 --include-tag ciGroup2
Details
X-Pack Chrome Functional tests / Group 3 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-3 --include-tag ciGroup3
Details
X-Pack Chrome Functional tests / Group 4 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-4 --include-tag ciGroup4
Details
X-Pack Chrome Functional tests / Group 5 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-5 --include-tag ciGroup5
Details
X-Pack Chrome Functional tests / Group 6 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-6 --include-tag ciGroup6
Details
X-Pack Chrome Functional tests / Group 7 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-7 --include-tag ciGroup7
Details
X-Pack Chrome Functional tests / Group 8 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-8 --include-tag ciGroup8
Details
X-Pack Chrome Functional tests / Group 9 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-9 --include-tag ciGroup9
Details
X-Pack Jest node --max-old-space-size=6144 scripts/jest --ci --verbose --detectOpenHandles
Details
X-Pack Karma Tests yarn test:karma
Details
X-Pack SIEM cyclic dependency test node legacy/plugins/siem/scripts/check_circular_deps
Details
X-Pack accessibility tests node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana --config test/accessibility/config.ts
Details
elasticsearch-ci/docs Build finished.
Details
eslint node scripts/eslint --no-cache
Details
kibana-ci Build finished.
Details
prbot:outdated
prbot:release note labels
prbot:release version labels
sasslint node scripts/sasslint
Details
@dgieselaar dgieselaar deleted the dgieselaar:error-group-alert branch Mar 24, 2020
dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Mar 24, 2020
* Add alerting/actions permissions for APM

* Export TIME_UNITS, getTimeUnitLabel from triggers actions UI plugin

* Add APM alert types and UI

* Review feedback

* Use Expression components for triggers

* Update alert name for transaction duration

* Change defaults for error rate trigger
dgieselaar added a commit that referenced this pull request Mar 24, 2020
* Add alerting/actions permissions for APM

* Export TIME_UNITS, getTimeUnitLabel from triggers actions UI plugin

* Add APM alert types and UI

* Review feedback

* Use Expression components for triggers

* Update alert name for transaction duration

* Change defaults for error rate trigger
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 24, 2020
* master:
  Updating our direct usage of https-proxy-agent to 5.0.0 (elastic#58296)
  allow users to unset the throttle of an alert (elastic#60964)
  [Lens] Fix bug in metric config panel (elastic#60982)
  [SearchProfiler] Minor fixes (elastic#60919)
  [ML] Renaming ML setup and start contracts (elastic#60980)
  introduce StartServicesAccessor type for `CoreSetup.getStartServices` (elastic#60748)
  [SIEM][Detection Engine] Add rule's notification alert type (elastic#60832)
  [APM] Re-revert "Collect telemetry about data/API performance" (elastic#61030)
  [NP] Graph: get rid of saved objects class wrapper (elastic#59917)
  [EPM] merge duplicate fields when creating index patterns (elastic#60957)
  [Uptime] Ml detection of duration anomalies (elastic#59785)
  [Alerting] removes unimplemented buttons from Alert Details page (elastic#60934)
  [skip-ci] Fix CODEOWNERS paths for the Pulse team (elastic#60944)
  [APM] Threshold alerts (elastic#59566)
  [ML] Add support for percentiles aggregation to Transform wizard (elastic#60763)
  Cahgen save object duplicate message (elastic#60901)
@cauemarcondes cauemarcondes self-assigned this Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.