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

Adds telemetry support to alerting and actions plugins #58081

Merged
merged 16 commits into from
Mar 12, 2020

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Feb 20, 2020

Summary

Close #49832

Checklist

Delete any items that are not applicable to this PR.

@YulNaumenko YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Feb 20, 2020
@YulNaumenko YulNaumenko requested a review from a team as a code owner February 20, 2020 03:08
@YulNaumenko YulNaumenko self-assigned this Feb 20, 2020
@elasticmachine
Copy link
Contributor

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

…telemetry

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@YulNaumenko YulNaumenko changed the title Adds telemetry support to alerting and actions plugins #49832 #54214 Adds telemetry support to alerting and actions plugins #49832 Feb 25, 2020
@YulNaumenko YulNaumenko changed the title Adds telemetry support to alerting and actions plugins #49832 Adds telemetry support to alerting and actions plugins Feb 25, 2020
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.

code LGTM, made some comments; will have a few more comments outside of the review, in the main part of the PR

x-pack/plugins/actions/server/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/actions/server/usage/actions_telemetry.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/mappings.json Outdated Show resolved Hide resolved
@@ -162,6 +170,22 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
actionsConfigUtils,
});

const usageCollection = plugins.usageCollection;
if (usageCollection) {
core.getStartServices().then(async ([coreStart, startPlugins]: [CoreStart, any]) => {
Copy link
Member

Choose a reason for hiding this comment

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

oh neat, didn't realized you could get start services (async) in setup!

taskManager: TaskManagerStartContract
) {
return usageCollection.makeUsageCollector({
type: 'actions',
Copy link
Member

Choose a reason for hiding this comment

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

so does usageCollection take type: actions and then look for actions-telemetry types in the SO? I'm not seeing any other references to actions-telemetry in the code, other than in the SO mappings at the top, so guessing this must be how it's done.

x-pack/plugins/actions/server/usage/task.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/server/alerts_client.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/server/usage/alerts_telemetry.ts Outdated Show resolved Hide resolved
…telemetry

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@mikecote mikecote self-requested a review February 28, 2020 13:29
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.

It's coming along, great work! 👍 Just a few comments, some of them are duplicates between alerts and actions and then a few nits.

x-pack/plugins/actions/server/usage/task.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/server/usage/task.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/server/usage/alerts_telemetry.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/server/usage/alerts_telemetry.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/server/usage/alerts_telemetry.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/server/usage/alerts_telemetry.ts Outdated Show resolved Hide resolved
x-pack/plugins/actions/server/usage/actions_telemetry.ts Outdated Show resolved Hide resolved
…telemetry

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
…telemetry

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@YulNaumenko YulNaumenko requested a review from mikecote March 12, 2020 04:58
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.

This is awesome! LGTM 👍I like how the number of requests is reduced.

Just wanted to confirm we have follow up issues created for the missing telemetry items? (executions, usage count, etc)

…telemetry

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
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, made some comments, seems fine to ship as-is

import { APICaller } from 'kibana/server';

export async function getTotalCount(callCluster: APICaller, kibanaIndex: string) {
const scriptedMetric = {
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly surprised we can't use some existing agg here, instead of having to script it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing aggregations is designed to work on the document fields, but in case it is a child document like a relation or alert.actions list - we should use nested path and in this case we still able to aggregate only fields on the nested level. I did a deep research over documentation and didn't find any other option https://www.elastic.co/guide/en/elasticsearch/reference/7.6/search-aggregations-bucket-nested-aggregation.html. The most pain here that we don't have an actionTypeId in 'relations', but have it on alert.actions and both of this objects are the separate branches of nesting which is not accessible for each other.

};
}

export async function getInUseTotalCount(callCluster: APICaller, kibanaIndex: string) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we could probably do all these aggs in a single search, but I doubt there's a great reason to do that, since they won't be running all that often. And it no doubt easier to deal with a small set of semi-complex searchs, rather than run a single search with a bunch of semi-complex aggs in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same thoughts, when was writing this code, but decided to keep it separately for now.

@@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import * as React from 'react';
import { mountWithIntl } from 'test_utils/enzyme_helpers';
import { mountWithIntl, nextTick } from 'test_utils/enzyme_helpers';
Copy link
Member

Choose a reason for hiding this comment

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

Are the changes in this file required for the PR? Like, the server plugin wasn't completely initialized before, or something?

Copy link
Contributor Author

@YulNaumenko YulNaumenko Mar 12, 2020

Choose a reason for hiding this comment

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

Yeah, it is the last weird test I found. Decided to fix it, because it cause jest tests failing with time out.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@YulNaumenko YulNaumenko merged commit d3f53f6 into elastic:master Mar 12, 2020
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Mar 12, 2020
* Added base telemetry functionality

* Fixed actions collector due to NP plugin changes

* Fixed type checks

* Fixed alerting plugin usage collector

* Added actions and alerting telemetry tasks

* Fixed failing tests and resolved comments

* Extended telemetry for alerts by adding aggregations for throttle, schedule and connectors count

* Fixed tests

* Refactored using callCluster aggregations

* Fixed compare

* Fixed time convertion
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 12, 2020
* master:
  Endpoint: Change the input type for @kbn/config-schema to work with more schemas (elastic#60007)
  Using re2 for Timelion regular expressions (elastic#55208)
  [Monitoring] Re-enable logstash tests (elastic#59815)
  fix karma debug typo (elastic#60029)
  Adds telemetry support to alerting and actions plugins (elastic#58081)
YulNaumenko added a commit that referenced this pull request Mar 12, 2020
* Added base telemetry functionality

* Fixed actions collector due to NP plugin changes

* Fixed type checks

* Fixed alerting plugin usage collector

* Added actions and alerting telemetry tasks

* Fixed failing tests and resolved comments

* Extended telemetry for alerts by adding aggregations for throttle, schedule and connectors count

* Fixed tests

* Refactored using callCluster aggregations

* Fixed compare

* Fixed time convertion
@mikecote mikecote mentioned this pull request Mar 16, 2020
3 tasks
avg: 0,
max: 0,
},
schedule_time: {
Copy link
Contributor

Choose a reason for hiding this comment

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

After checking off line with @YulNaumenko , the mapping for the schedule_time and throttle_time properties should be strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Actions 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.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Telemetry & KPI's for beta, to be defined
6 participants