search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.7k stars 8.24k forks source link

Validate alert / action types as they get registered #49399

Closed mikecote closed 3 years ago

mikecote commented 5 years ago

Currently we're relying on TypeScript to find errors within the alert / action types. We should use Joi or config-schema to do the validation within the register function.

elasticmachine commented 5 years ago

Pinging @elastic/kibana-stack-services (Team:Stack Services)

pmuellr commented 5 years ago

How is this different than the configuration we already do? Eg https://github.com/elastic/kibana/blob/dad9d6b1b97b6712c2b45257eb5be9980ffdbd18/x-pack/legacy/plugins/actions/server/builtin_action_types/pagerduty.ts#L22-L68

I believe it's true that you don't need to provide a validator function, and we probably haven't fixed on the exact interface yet (but built-in actions use config-schema).

mikecote commented 5 years ago

@pmuellr This one would be to validate the structure of the action type / alert type instead of the structure config or params contains.

The validation would happen within alertTypeRegistry.register(...) and actionTypeRegistry.register(...) and validate the structure passed in. It would make sure it matches a structure like this: https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/alerting/server/types.ts#L37-L45.

Use case I'm thinking is the developers who don't use TypeScript yet (stack monitoring) and for us to have a way to notify them of structure changes we make. (ex: actionGroups now mandatory for alert types).

pmuellr commented 5 years ago

ya, this would be good

gmmorris commented 3 years ago

I think we should push for as much compile-time validation as possible, and given the wide adoption of TS in the Kibana codebase, I'm not sure I see what problem we're actually trying to address here.

Unless I'm missing something 🤔

pmuellr commented 3 years ago

In the big scheme of things, agree with @gmmorris, the one thing I'd rather spend time on re: validation is our reading and writing of Saved Objects.

gmmorris commented 3 years ago

the one thing I'd rather spend time on re: validation is our reading and writing of Saved Objects.

Yeah, we can definitely make those better. I ran an audit a while back adding fp-ts & io-ts validations when reading some of our SOs, but we could improve things for sure.

mikecote commented 3 years ago

I think we should push for as much compile-time validation as possible, and given the wide adoption of TS in the Kibana codebase, I'm not sure I see what problem we're actually trying to address here.

@gmmorris This issue was created before plugins migrated their code to TS and this would of been catching similar issues for those not using TS. I think it's safe to close this issue and rely on compile-time validation.

the one thing I'd rather spend time on re: validation is our reading and writing of Saved Objects.

I ran an audit a while back adding fp-ts & io-ts validations when reading some of our SOs, but we could improve things for sure.

If we still see an issue with data integrity, feel free to open one. I will close this one as it is scoped to the alert / action types.