Add AggTypeFilters to filter out aggs in editor #19913
Conversation
timroes
added
Feature:Visualizations
💔 Build Failed |
| * | ||
| * @param filter The filter to register. | ||
| */ | ||
| public register(filter: AggTypeFilter): void { |
There was a problem hiding this comment.
I would find this clearer to understand if the method was named addFilter -- register doesn't carry any implicit meaning for me, and it also raises some possible confusion with the registry system we currently use to communicate between plugins.
| * @param aggConfig The aggConfig for which the returning list will be used. | ||
| * @return A filtered list of the passed aggTypes. | ||
| */ | ||
| public filter$(aggTypes: AggType[], vis: Vis, aggConfig: AggConfig) { |
There was a problem hiding this comment.
I spent a lot of time reading this line by line to parse out what it was doing. I think if we changed some names and assigned some variables we can make the logic a little more accessible. Here's what I suggest:
public allowedAggTypes$(aggTypes: AggType[], vis: Vis, aggConfig: AggConfig) {
return this.subject.map(filters => {
const aggTypeFilters = Array.from(filters);
const allowedAggTypes = aggTypes.filter(aggType => {
const isAggTypeAllowed = aggTypeFilters.every(aggTypeFilter => aggTypeFilter(aggType, vis, aggConfig));
return isAggTypeAllowed;
});
return allowedAggTypes;
});
}| * and limits available aggregations based on that. | ||
| */ | ||
| aggTypeFilters.register((aggType: AggType, vis: Vis, aggConfig: AggConfig) => { | ||
| return filterByName([aggType], aggConfig.schema.aggFilter).length === 1; |
There was a problem hiding this comment.
I had to scratch my head for a few minutes to understand why we were checking for strict equality with 1. And then I realized that it was simply because there is a single element in [aggType]. 😄 I feel like this is a bit misleading (and unnecessary) because we really just care that the filter did not exclude the aggType, i.e. didn't return an empty array. It might be clearer to do this instead:
aggTypeFilters.register((aggType: AggType, vis: Vis, aggConfig: AggConfig) => {
const doesFilterAllowAggType = filterByName([aggType], aggConfig.schema.aggFilter).length !== 0;
return doesFilterAllowAggType;
});
|
Also, with the renaming of |
| @@ -40,7 +41,14 @@ uiModules | |||
| $scope.$bind('agg', attr.agg); | |||
| $scope.$bind('groupName', attr.groupName); | |||
|
|
|||
| $scope.aggTypeOptions = aggTypes.byType[$scope.groupName]; | |||
| const aggTypeSubscription = aggTypeFilters | |||
There was a problem hiding this comment.
How important/valuable is it that we're exposing observables through the interface here? For example, I can also picture this functionality being supported by an interface that looked like this (while still using observables internally):
const aggTypeSubscription = new AggTypeSubscription(aggTypes.byType[$scope.groupName], $scope.vis, $scope.agg);
aggTypeSubscription.subscribe(aggTypes => $scope.aggTypeOptions = aggTypes);This would encapsulate the complexity of observables and present me with an interface which appears and behaves like an event emitter.
There was a problem hiding this comment.
If we are anyway working with observables I think we are actually also want to expose this for a couple of reasons:
- Otherwise we also need to build deregistration logic in that interface.
- Observables are super powerful and allow tons of operators, so if the actual consumer of that API need to postprocess the answer in anyway or want to debounce it, or just listen for the first changes, etc. - there is already an Observable operator for that. I don't see a good reason why we would want to deliberately kill that flexibility and possibly force the user to wrap everything in an observable again to be able to achieve that operations.
Since Observables and RxJS tend to become a common pattern (and tend to become a standard JavaScript pattern) I would rather only hide Observables in places we use them, if there is a really good reason for doing so.
There was a problem hiding this comment.
I have a couple thoughts in response, but keep in mind I'm coming to this with zero experience in observables, so I'm mostly asking questions to learn and not with any strongly held opinion. I would not consider my comments a blocker on this PR in any way.
Observables are super powerful and allow tons of operators
100% agree, from what I've learned about them so far their power is apparent. But this is why I'm trying to learn more about why this power is necessary, since you don't want to use a tank when all you need is a bicycle. In this particular case, it doesn't seem like we're taking advantage of the observable. Are there concrete use cases for the aggTypeFilters module in which this power is necessary on the consumer's side, or examples of similar modules elsewhere that make the advantages more obvious?
I don't see a good reason why we would want to deliberately kill that flexibility
Just to explain my thought process, I look for the abstractions we're creating in our code and try to figure out what kind of model can be inferred from them. So I poke and probe at abstractions that are complex and try to understand if that complexity is necessary. An example of where unnecessary complexity in abstractions created a confusing model would be our courier code. I was thinking that if it turns out that the observable is really just an implementation detail, then it might make sense to reduce complexity for the consumer by hiding it.
Since Observables and RxJS tend to become a common pattern
No argument here. 😄 I agree that observables can be really useful and I'm sure there are situations where an abstraction that deals in observables makes sense. I'm just trying to understand why they're a core part of this abstraction's interface. I have very little experience with observables, so I'm really just trying to learn more about the situations in which they're most useful and understand how this is one of those situations.
|
@trevan Since I saw you liked that PR: do you any specific use-case were this will come in handy? We'll still add extension points to filter down available fields and to set some options (like histogram interval) to fixed values or disable options altogether (like Other Bucket for Terms, which won't work in Roll-ups). So if you already have some use-cases (in mind) were you would require some of this functionality, I would love to hear about them. If it's too long to quickly describe those in a PR, we could also setup a virtual meeting. |
💔 Build Failed |
|
@timroes, some use cases that I have:
|
💔 Build Failed |
|
That's some interesting use-cases. 2 will be handled by the follow up PR to limit fields. 1 would potentially fall in the responsibility of this PR, but unfortunately this is not really possible, since we "and" all filters currently, so you cannot add another filter, that would allow an aggregation that has been filtered out by a different filter. Do you have a suggestion how we should extend the API to allow that use-case? 3 won't be handled by any of those PR I fear, since we won't change the bucket/metric association as it's currently is. I also think we won't touch that topic anymore for the current editor, that we want to replace in the future. |
|
Blocked by issues using RxJS 5 with TypeScript currently. Will need to figure out if we can get RxJS 6 update done soon. (#18885) |
|
For 1, you could create an "add" filter list and a "remove" filter list. It would first go through all of the "add" filters and create a list of aggs to use. Then it would run the "remove" filters and trim the list. It loos like all the existing aggFilters are either purely inclusion or exclusion. So you could split the aggFilter into "aggFilterInclusive" and "aggFilterExclusive". You'd create an "add" filter out of the aggFilterInclusive items and create a "remove" filter out of the aggFilterExclusive items. I think that would then allow developers to add additional aggs and/or remove unwanted aggs. |
💚 Build Succeeded |
|
@trevan Thanks for that feedback. I have the feeling, that would kind of lead to a complicated situation at the moment, since we already allow the aggFilter themselves on the visualization type to be inclusive or exclusive and then having two separate filter registries for filters. Also a lot of that stuff will change a) with the new editor we are working on and b) with using the canvas pipeline for all visualizations to render - since with that only output and input of specific functions in the pipeline will matter. So I would tend not to implement this right now with this PR, but keep all 2 outstanding points (the field one will anyway be made in one of the next PRs), as input for the new editor and pipeline, that we'll make that requirements possible in those. |
|
@trevan The new editor will be a replacement of the current one, so it will be OSS. I will look in our next meeting, that we find a way to provide mockups already with the community. We are currently in a very early stage of the UI part of it, and need to figure out the best way for that. |
💚 Build Succeeded |
|
@cjcenizal After talking to Peter we decided to pass in |
💔 Build Failed |
|
jenkins, test this |
💔 Build Failed |
|
@timroes, I'd like to have the vis passed in or at least the type of the vis (pie, tile_map, table, etc). That way, I can have logic to determine if I show/hide an agg depending on the vis type. |
|
@timroes, also, could you possibly allow extensions to clear the filters list or be able to remove specific filters? If I could remove the default one that is created, I could then do my use case 1. |
|
Thanks @timroes! I pulled this down and am able to limit aggregation types based on rollup capabilities. Rollups will also need to limit fields based on the aggregation type. For example, I believe passing |
|
@jen-huang yeah I splitted that up into three different PRs, we need one for aggregation types, one for field types (will have around the same size) and one to limit down editor options to either hide them (like Other bucket and missing for terms) or set them to a fixed values (like (date) histogram intervals). Also we'll need some refactoring to be able to get access to the index pattern when writing the aggregation configuration, since we need to set a fixed timezone inside the If you already have capacity for doing the field one, that would of course be nice, since this is pretty straight forward, whereas the parameter and date histogram are a bit more tricky. So I could already work on them and wouldn't need to spend time in the field limiting one in case you've got capacity for that. @trevan After thinking a bit about the discussion, and since you already have a working solution, we rather won't fix this PR anymore, so you could implement the first use-case. I am sorry, that we won't make it with this one, but since all of that code will be going away in the long run in favor of the new editor, we rather not spend too much time in it now, and hope we can find a solution with the new infrastructure in general, that you won't need to modify any Kibana sources anymore. |
💚 Build Succeeded |
| * registered with this registry will change. | ||
| * | ||
| * @param aggTypes A list of aggTypes that will be filtered down by this registry. | ||
| * @param vis The vis for which this list should be filtered down. |
There was a problem hiding this comment.
should be changed to indexPattern
💚 Build Succeeded |
* Add AggTypeFilters to filter out aggs in editor * Add documentation * Implement CJ's feedback * Add link to missing vis variable * Fix for RxJS 6 * Add babel-core types and fix tests * Pass index pattern instead of vis * Fix documentation
* Add AggTypeFilters to filter out aggs in editor * Add documentation * Implement CJ's feedback * Add link to missing vis variable * Fix for RxJS 6 * Add babel-core types and fix tests * Pass index pattern instead of vis * Fix documentation
* Add AggTypeFilters to filter out aggs in editor * Add documentation * Implement CJ's feedback * Add link to missing vis variable * Fix for RxJS 6 * Add babel-core types and fix tests * Pass index pattern instead of vis * Fix documentation
This PR introduces
AggTypeFilterswhich allow to register filters to a central registry, which will get a list of all aggregations type and can filter them depending on the vis and aggConfig.That will be required for roll-up support to allow registering a filter, that checks if an index pattern is a rolled up index pattern and limit available aggregations on those.
It also began introducing typings for the core classes we need. Even if a lot of types are still set to
anyI want to introduce them as early as possible, since we can then use those types already in all TS files and as soon as we refine the types to match reality more, we actually are able to check, if everything still works (or works as expected).I also moved our current logic to filter aggregations based on what the schema definition in the vis type requires into the new system and remove it from the Angular world.