-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[data.search.aggs]: Create agg types function for terms agg. #63541
[data.search.aggs]: Create agg types function for terms agg. #63541
Conversation
b028380
to
f6e1723
Compare
Pinging @elastic/kibana-app-arch (Team:AppArch) |
export interface AggExpressionType { | ||
type: 'agg_type'; | ||
value: CreateAggConfigParams; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the shape of the agg_type
which every agg type function returns. That way we can enforce that esaggs
only accept agg_type
as an argument, instead of a stringified JSON blob.
export type AggExpressionFunctionArgs< | ||
Name extends keyof AggParamsMapping | ||
> = AggParamsMapping[Name] & Pick<AggConfigJson, 'id' | 'enabled' | 'schema'>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generic helps to streamline the process of typing arguments for an expression function. It uses the provided Name
to retrieve the params for the agg from the AggParamsMapping
below. Then it combines those with id
, enabled
, and schema
from the agg config options -- these arguments will be required for each of the agg type expression functions, however the params will be different between each.
export interface AggParamsMapping { | ||
terms: AggParamsTerms; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add each new agg type here, along with the params for that agg. This way you can access all the agg params from this mapping.
Eventually we can make this interface public, and folks could even add to it using declare module
to append their agg to the interface. (This would be necessary if, for example, there were a special type of aggregation which was only provided by x-pack).
f6e1723
to
83f7a8f
Compare
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need a way to get from aggConfig to expression function. once we have expression builder ready:
const as = agg.toJSON();
createExpression().addFunction(as.type, as.params)
can we assume that serialized params are exact params to the expression function ?
with terms the problem is with orderAgg parameter, which in expression function should be a subexpression. if serialized params and expression function params are different we will need to extend agg type to return expressionparams :|
how do you feel about adding .toAST()
function to aggConfig ?
aggTypes.buckets.forEach(b => aggTypesSetup.registerBucket(b)); | ||
aggTypes.metrics.forEach(m => aggTypesSetup.registerMetric(m)); | ||
|
||
// register expression functions for each agg type | ||
const aggFunctions = getAggTypesFunctions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you tried to do it in a way like we do for aggTypes
. But maybe we can create a factory method (or you can use some other pattern) to encapsulate forEach methods and make the setup hook easier for reading. It will be awesome if you will do it for aggTypes
and aggFuncitons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexwizp Just to make sure I follow; how would you envision this looking? Just removing the forEach
from search_service
entirely & passing expressions
down so it is handled inside of the function instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I ask is because readability is what I was aiming for here, but of course I'm always open to ideas to make things more readable. Rather than having the magic of stuff getting registered in another function, I liked the idea of seeing very clearly what this service was registering during setup
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a start I removed the bucket/metric differentiation per Peter's suggestion; lmk what your thoughts are and I'm happy to adjust further
@@ -60,6 +60,21 @@ export interface TermsBucketAggDependencies { | |||
getInternalStartServices: GetInternalStartServicesFn; | |||
} | |||
|
|||
export interface AggParamsTerms { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear enough why you created that interface in buckets/terms.ts
not in buckets/terms_fn.ts
From my point of view we can simplify your approach and remove AggExpressionFunctionArgs
and AggParamsMapping
types at all. I don't see any benefits for today.
what about to do all buckets/terms_fn.ts
:
....
import { AggExpressionType, AggConfigJson, IAggExpressionFunction } from '../';
const aggTypeName = 'terms';
const fnName = 'aggTypeTerms';
type Input = any;
type Output = AggExpressionType;
export interface AggParamsTerms extends IAggExpressionFunction {
field: string;
order: 'asc' | 'desc';
orderBy: string;
orderAgg?: AggConfigJson;
size?: number;
missingBucket?: boolean;
missingBucketLabel?: string;
otherBucket?: boolean;
otherBucketLabel?: string;
// advanced
exclude?: string;
include?: string;
}
type FunctionDefinition = ExpressionFunctionDefinition<
typeof fnName,
Input,
AggParamsTerms,
Output
>;
export const aggTypeTerms = (): FunctionDefinition => ({
....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the reason I put this with terms.ts
& created the AggParamsMapping
was because I felt that having typings for each of the agg type params would be generally useful outside of the expression functions & didn't have anything to do with expressions in particular.
For example, say I want to create a new agg config. Without the mapping, I could do:
const configState = {
type: 'terms',
enabled: true,
params: {
field: 'machine.os.keyword',
// ...etc
}
};
data.search.aggs.createAggConfigs(indexPattern, [configState]);
But configState
is only enforced to match the general shape of params (currently pretty useless: Record<string, any>
). What would be ideal would be to have the ability to pull in an interface for the params you are using so that you can have more type safety in your params. Perhaps by making the serialized agg config interface a generic:
const configState: AggConfigSerialized<AggParamsTerms> = {
type: 'terms',
enabled: true,
params: {
field: 'machine.os.keyword',
// ...now this would be more strictly typed
}
};
Or even better, using the type from the global AggParamsMapping
based on the type name string.
More concretely, as we have an agg types registry which technically could have anything registered to it, having a mapping of these interfaces that others can add to via declare module
will be necessary eventually anyway.
Oops, totally forgot about this, thanks for the reminder.
IMHO we should start with a separate little utility function that takes in the aggConfig & returns the expression AST. This would be easy to remove later when the expression builder is ready, at which point folks can just use
That is my assumption. The expression args are serialized params (which differ on a per-agg basis), and enabled, id, + schema. As you pointed out, I think it'd be problematic/confusing if there was a divergence between expression args & params. This is why I'd like to eventually change how we are typing the agg params so that they are using the same interface as the expression fn is using for the args. (Edit: Also a param of type
I'm looking into this, but even if it were necessary i think it would be okay, since the subexpression could just be another agg type expression function with the serialized info for that agg. |
37b7af7
to
09d97e5
Compare
…function ast instead of expression ast.
6296718
to
0588f1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM, left two small comments. once we have AST builder ready this can be cleaned up (should probably the first case for using AST builder).
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…#63541) * Minor cleanup on AggConfigJson interface. * Add types for terms agg params & mapping. * Add terms agg expression function. * Register agg functions with expressions service. * Add unit tests for terms expression function. * Update API docs. * AggConfigJson -> AggConfigSerialized * Add serialize(), enforce serializable params, fix subexpressions in terms agg fn. * Simplify getAggTypesFunctions * it() -> test() * Add help text. * Ensure serialize() is used by agg param type. * Add toExpressionAst to AggConfig. * Add json arg to terms agg fn. * Update docs. * Fix typo which caused functional test failures. * Add AggParam.toExpressionAst so AggConfig.toExpressionAst can return function ast instead of expression ast. * Clean up overlooked items.
* master: (70 commits) KQL removes leading zero and breaks query (elastic#62748) [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193) [ML] Changes transforms wizard UI text (elastic#64150) [Alerting] change server log action type .log to .server-log in README (elastic#64124) [Metrics UI] Design Refresh: Inventory View, Episode 1 (elastic#64026) chore(NA): reduce siem bundle size using babel-plugin-transfor… (elastic#63269) chore(NA): use core-js instead of babel-polyfill on canvas sha… (elastic#63486) skip flaky suite (elastic#61173) skip flaky suite (elastic#62497) Renamed ilm policy for event log so it is not prefixed with dot (elastic#64262) [eslint] no_restricted_paths config cleanup (elastic#63741) Add Oil Rig Icon from @elastic/maki (elastic#64364) [Maps] Migrate Maps embeddables to NP (elastic#63976) [Ingest] Data streams list page (elastic#64134) chore(NA): add file-loader into jest moduleNameMapper (elastic#64330) [DOCS] Added images to automating report generation (elastic#64333) [SIEM][CASE] Api Integration Tests: Configuration (elastic#63948) Expose ability to check if API Keys are enabled (elastic#63454) [DOCS] Fixes formatting in alerting doc (elastic#64338) [data.search.aggs]: Create agg types function for terms agg. (elastic#63541) ...
related #61768, #61769
Summary
First pass at creating an agg type function. I went with terms because it is one of the more complex ones. Once we decide we are happy with this approach, we'll create functions for all other aggs, and then refactor
esaggs
to use them.The goal here is to make sure we are comfortable with the general approach, before we repeat it for a bunch of aggs 😄
I tried to implement this in a way that makes it as easy as possible to "scale" and add new functions, since we will need to add a bunch of them (one for each agg type). At this point adding a new function looks like this:
AggParamsMapping
intypes.ts
agg_types.ts
and add it togetAggTypesFunctions
cc @ppisljar @alexwizp