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

Expose data service in the alert execution method #87990

Closed
timroes opened this issue Jan 12, 2021 · 9 comments
Closed

Expose data service in the alert execution method #87990

timroes opened this issue Jan 12, 2021 · 9 comments
Labels
enhancement New value added to drive a business result estimate:small Small Estimated Level of Effort Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Feature:Alerting NeededFor:DataDiscovery NeededFor:VisEditors Project:MoreRuleTypes Alerting team project for providing more ways to construct rules. Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@timroes
Copy link
Contributor

timroes commented Jan 12, 2021

Currently you only get a scoped elasticsearch client and saved object client passed into the services of the executor method of an alert type. Since we try to encourage usage of the data plugin and its services across Kibana it would be good if we could get that injected too as a service.

Also to scope that at the moment you'll require a KibanaRequest (for the same reasons, that most likely the alerting framework is forging one internally - that most core services don't work without one). Since the alerting framework is anyway forging one it could scope that service as it is. Later on ideally for scoping we'd only need scoped clients, but that will require a lot of more core work, and could then internally be refactored inside the alerting framework without any alert type need adjustments. Previous discussion on why the KibanaRequest is needed right now can be found in #78461

This issue is currently blocking Alerting in core Kibana apps like Discover.

cc @lukeelmers @ppisljar

@timroes timroes added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) NeededFor:KibanaApp labels Jan 12, 2021
@elasticmachine
Copy link
Contributor

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

@lukeelmers
Copy link
Member

Previous discussion on why the KibanaRequest is needed right now can be found in #78461

In that linked discussion, we had gone back and forth on whether to only provide scoped services in our server APIs, or to provide the entire KibanaRequest object, so this another argument in favor of removing KibanaRequest from those APIs entirely.

The main reason the app team is blocked on this is because:

  • High level search (SearchSource) requires a full KibanaRequest object on the server, because:
  • Low level search requires a full KibanaRequest object, because:
  • Search strategies are given a full KibanaRequest object

So I think the only way to unblock this would be for search strategies to make a breaking change and only provide a predefined list of scoped services, rather than the full request object. Then low-level search could pass these services in, and high-level search could also be updated to only require those services.

At the moment search source would just require ES client & SO client in order to function on the server, so if this work were done no further changes would be required from the alerting service AFAICT... though it still might be nice to still expose data as a convenience.

@mikecote
Copy link
Contributor

mikecote commented Feb 5, 2021

Moving from 7.x - Candidates to 7.13 after the latest 7.x planning session. If we get time, we will implement this.

@mikecote
Copy link
Contributor

If we provide the KibanaRequest to the executor, would that suffice? and you use your data plugin object from your plugin?

@timroes
Copy link
Contributor Author

timroes commented Apr 26, 2021

@mikecote yes, technically that would be enough. Though I want to raise the question if we wouldn't rather want to inject the data service. We're trying to establish most of those services as fundamentals across Kibana for all apps (index patterns, search source, expressions, etc.) If we consider that app Services wants them to be used across Kibana as much as possible and we're trying to move apps over usnig them, it feels like we should try to give access to the data plugin more directly in alerts, and not require everyone to intantiate this from faked KibanaRequest themselves in their alerters. cc @stacey-gammon

@ppisljar
Copy link
Member

@timroes you can't inject the data service (well you can, but not scoped). You would need to inject specific parts of it scoped to the right user, so that's a lot of dependencies (search, searchsource, indexpatterns). I think passing in KibanaRequest is way more flexible and allows plugins to scope more.

@mikecote
Copy link
Contributor

I did a quick POC last week and came to the same conclusion as @ppisljar. There wasn't a way to scope the entire start contract of the data service to the user automatically. An extra step would be necessary (by the alert type author) to scope the search, searchsource, indexpattern service to the user with a request.

We've been getting a few requests to pass the request object down to the executor function, which isn't a problem. Passing the data service as well isn't a problem but connecting the two isn't something we can do unless we start developing our own interface to the data service. Thoughts?

@timroes
Copy link
Contributor Author

timroes commented Apr 27, 2021

Given what Peter mentioned it sounds maybe like the better solution to actually just pass the request and leave the scoping of the appropriate services to the individual plugins in this case.

@gmmorris gmmorris added Feature:Alerting Project:MoreRuleTypes Alerting team project for providing more ways to construct rules. Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework labels Jun 30, 2021
@gmmorris gmmorris added the loe:medium Medium Level of Effort label Jul 15, 2021
@gmmorris gmmorris added enhancement New value added to drive a business result estimate:small Small Estimated Level of Effort labels Aug 13, 2021
@gmmorris gmmorris removed the loe:medium Medium Level of Effort label Sep 2, 2021
@ymao1
Copy link
Contributor

ymao1 commented Nov 19, 2021

Closing as no longer needed. Feel free to open a PR if you need access to the request object.

@ymao1 ymao1 closed this as completed Nov 19, 2021
@kobelb kobelb added the needs-team Issues missing a team label label Jan 31, 2022
@botelastic botelastic bot removed the needs-team Issues missing a team label label Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result estimate:small Small Estimated Level of Effort Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Feature:Alerting NeededFor:DataDiscovery NeededFor:VisEditors Project:MoreRuleTypes Alerting team project for providing more ways to construct rules. Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
None yet
Development

No branches or pull requests

8 participants