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.

Sign up for GitHub

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

Jump to bottom

[Usage Collection] [schema] alerts #78933

Merged
merged 5 commits into from
Oct 2, 2020
Merged

[Usage Collection] [schema] alerts #78933

merged 5 commits into from
Oct 2, 2020

Conversation

afharo
Copy link
Member

@afharo afharo commented Sep 30, 2020 •
edited
Loading

Summary

Add schema definition to the collector alerts.

Using DYNAMIC_KEY as a way to tell when the key can be dynamic, and we can't know the possible values.

Related to #70180.

For maintainers

@afharo afharo added Feature:Telemetry Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:KibanaTelemetry Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.10.0 review labels Sep 30, 2020
afharo
afharo commented Sep 30, 2020
View reviewed changes
x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json Outdated
Comment on lines 53 to 66
"count_active_by_type": {
"properties": {
"DYNAMIC_KEY": {
"type": "long"
}
}
},
"count_by_type": {
"properties": {
"DYNAMIC_KEY": {
"type": "long"
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as in #78832 @mindbat

@afharo afharo mentioned this pull request Sep 30, 2020
Closed
14 tasks
@afharo afharo force-pushed the usage_collection/schema/alerts branch from ee94cea to a65b7ce Compare September 30, 2020 13:32
@afharo afharo force-pushed the usage_collection/schema/alerts branch from a65b7ce to 2a45f05 Compare September 30, 2020 14:56
@afharo afharo marked this pull request as ready for review September 30, 2020 17:22
@afharo afharo requested review from a team as code owners September 30, 2020 17:22
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

TinaHeiligers
TinaHeiligers reviewed Sep 30, 2020
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Schema additions for known fields GTM. I'm on the fence about providing "DYNAMIC_KEY" types if we don't have known values.

x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json
},
"count_active_by_type": {
"properties": {
"DYNAMIC_KEY": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as comment #78832 (comment)
Having a placeholder for unknowns is a great way to keep track of fields that are probably unmapped right now. If Infra isn't comfortable with filtering out the DYNAMIC_KEY fields, we'll have to take care of that in the diff we provide at FF (i.e. ignore these fields because they can't be mapped in the legacy service indices anyway).

mikecote
mikecote reviewed Oct 1, 2020
View reviewed changes
x-pack/plugins/alerts/server/usage/alerts_usage_collector.ts Outdated
avg: { type: 'float' },
max: { type: 'long' },
},
// TODO: Find out all the possible values for DYNAMIC_KEY or reformat into an array
Copy link
Contributor

@mikecote mikecote Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Cross referencing a discussion from #78832 (comment) in regards to if these can be gathered somehow. For this one, plugins do call registerType for their own types of alerts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @mikecote! I'll run through the code to find all the known registered types.

Do you think we can still keep the extra DYNAMIC_KEY entry to let the Remote Telemetry team know that it might grow in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! I've added the known registered alerts. We might need to re-run this search after each FF to keep it up to date thought (or find out a way to populate it automatically, or reformat that key into an array the Remote Service can deal with).

Bamieh
Bamieh approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

seems like a good workaround for this issue to fix the schema.

TinaHeiligers
TinaHeiligers approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

LGTM with the known fields.

mikecote
mikecote approved these changes Oct 2, 2020
View reviewed changes
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.

Alerting changes LGTM!

x-pack/plugins/alerts/server/usage/alerts_usage_collector.ts
import { get } from 'lodash';
import { TaskManagerStartContract } from '../../../task_manager/server';
import { AlertsUsage } from './types';

const byTypeSchema: MakeSchemaFrom<AlertsUsage>['count_by_type'] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mindbat same question as #78832 (comment).

@mindbat did you want the alerting team to keep updating this list on each release when new actions exist? or will it be possible with the DYNAMIC_KEY to automatically handle new types of actions per release on your end?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@afharo afharo merged commit 7afb8b4 into elastic:master Oct 2, 2020
@afharo afharo deleted the usage_collection/schema/alerts branch October 2, 2020 16:45
@afharo afharo mentioned this pull request Oct 2, 2020
Merged
afharo added a commit that referenced this pull request Oct 2, 2020
@afharo
[7.x] [Usage Collection] [schema] alerts (#78933) (#79332)
fa7d1a9
@lukeelmers lukeelmers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 1, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@Bamieh Bamieh Bamieh approved these changes

@mikecote mikecote mikecote approved these changes

Assignees
No one assigned
Labels
Feature:Alerting Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@afharo @elasticmachine @kibanamachine @mikecote @Bamieh @TinaHeiligers @lukeelmers