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

[Usage Collection] [schema] actions #78832

Merged
merged 3 commits into from
Oct 2, 2020

Conversation

afharo
Copy link
Member

@afharo afharo commented Sep 29, 2020

Summary

Add schema definition to the collector actions.

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

Comment on lines 20 to 22
"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.

@mindbat for now, when there is a dynamic key with unknown values, schema will be reported with DYNAMIC_KEY. Would that work for you?

Copy link

Choose a reason for hiding this comment

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

I'm afraid I don't understand the question. This is the schema internal to Kibana? So there's no direct correspondence to what gets sent in the object to be indexed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mindbat that's not quite true. The schema is a way of indicating what is sent in the object and the field type we can expect to map that as (long vs float, date, keyword etc). The schema won't necessarily correspond directly to the index mapping but it will indicate the value type for that field.

That being said, the point of introducing schemas was to provide known fields and their expected types. We (from the Kibana side) shouldn't provide anything that we, um, don't know!

@afharo we'll still need to update mappings for fields we do know and are certain about and I'm we're highly unlikely to be maintaining mappings for dynamic fields now anyway (how could we if we don't know what they are 🤷‍♀️ ).

@mindbat Would you be ok with perhaps ignoring the mappings for these types of "DYNAMIC_KEY" fields? i.e. we maintain and update diffs in the fields we know but don't concern ourselves (i.e. don't map them at all) with those that we don't?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mindbat everything reported in the schema is directly correlated to the data that gets shipped under stack_stats.kibana.plugins.

Unfortunately, we've still got a few objects whose keys are dynamic and we need the schema to describe them in some way. Then it's up to the Remote Telemetry Service to map them via Dynamic Mapping Templates, change their structure in the Indexer, or don't index/map them at all. Our take up until now has been: index them but don't map them (so they are in the _source for advanced visualisations like Vega, but can't really be searched/aggregated).

The question is: is DYNAMIC_KEY a valid indicator for you to identify these type of keys?

FYI: Based on the discussion in here (#78832 (comment)), we can provide some of the already-known keys. But the list is likely to grow dynamically (and we'll try to keep it up to date). Should, in those cases, still provide the known keys and the extra DYNAMIC_KEY for you to be aware of that case?

@afharo afharo marked this pull request as ready for review September 29, 2020 19:38
@afharo afharo requested review from a team as code owners September 29, 2020 19:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@elasticmachine
Copy link
Contributor

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

@afharo afharo requested a review from mindbat September 30, 2020 11:49
@afharo afharo mentioned this pull request Sep 30, 2020
1 task
Comment on lines 22 to 24
// TODO: Find out all the possible values for DYNAMIC_KEY or reformat into an array
count_by_type: { DYNAMIC_KEY: { type: 'long' } },
count_active_by_type: { DYNAMIC_KEY: { type: 'long' } },
Copy link
Contributor

@mikecote mikecote Sep 30, 2020

Choose a reason for hiding this comment

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

It looks like we can get a list of types here since the collector is created after setup phase (https://github.com/elastic/kibana/blob/master/x-pack/plugins/actions/server/plugin.ts#L228). Did you need some help to access the list of types?

Also not sure how this would then correlate to x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also not sure how this would then correlate to x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json.

We run the command node scripts/telemetry_extract.js to find all the schema declarations and merge them into that file.

It looks like we can get a list of types here since the collector is created after setup phase (master/x-pack/plugins/actions/server/plugin.ts#L228). Did you need some help to access the list of types?

Unfortunately, the command mentioned above doesn't run Kibana. It scans the code instead. But if you can point me to way other plugins register actions, I can search them and populate the current list. We'll likely need to refresh the list on every FF, until we find a way to automate it. Is it just looking for registerType(ACTION_TYPE)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it just looking for registerType(ACTION_TYPE)?

I've looked for those and I couldn't find any use of those (only in test plugins) 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it would be because only the actions plugin registers action types at this time. They are all defined here but call register(...) which is a lower level function.

https://github.com/elastic/kibana/blob/master/x-pack/plugins/actions/server/builtin_action_types/index.ts#L30-L38

Interesting about scanning the code. You will find plugins registering alert types but we don't have any yet registering action types (in 1-2 release from now maybe).

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikecote thank you for the pointer! I'll update the list with the known keys.

Similar to the question in alerts, do you think it's still a good idea to maintain the DYNAMIC_KEY to let the Remote Telemetry cluster know that it might grow (just in case they want to set up a dynamic mapping template of some sort)?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikecote done! I just pushed the updated schema with the list of known built-in actions. Thank you for pointing me to where to find them

@afharo afharo mentioned this pull request Sep 30, 2020
1 task
@afharo afharo force-pushed the usage_collection/schema/actions branch from d3a92cc to a55a9e5 Compare September 30, 2020 14:54
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.

can we map them to dynamic and ask the actions/alerting team to map those since they'd be able to get the list of keys?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

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

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!

import { get } from 'lodash';
import { TaskManagerStartContract } from '../../../task_manager/server';
import { ActionsUsage } from './types';

const byTypeSchema: MakeSchemaFrom<ActionsUsage>['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 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?

Copy link

@mindbat mindbat Oct 7, 2020

Choose a reason for hiding this comment

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

I'm a little fuzzy on the code here (not a ts dev 😅 ), but it's my understanding that @afharo's DYNAMIC_KEY in the schema that gets handed off to my team will be enough for us to know "this key's value will have this type, but its name might change"

@afharo afharo merged commit f398b49 into elastic:master Oct 2, 2020
@afharo afharo deleted the usage_collection/schema/actions branch October 2, 2020 15:03
@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
Labels
Feature:Actions 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 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants