[APM] Refactor & scope aggregation types

[dgieselaar]

Iterate on the APM aggregation types, make them more readable and explicitly scope them as APM-specific.

ES typings:

• Refactored aggregation types (mostly better naming, slightly less hacks)
• Do not add APM typings to elasticsearch module to explicitly scope them as APM specific
• Make restTotalHitsAsInt a type option rather than a separate type
• terms bucket key type now accounts for the fact it can be both number and string

mergeProjection:

• Type check arguments for compatibility with ESSearchRequest
• Require valid aggregation objects in source parameter. Seems less brittle, and surfaces errors closer to the source

Other changes:

• Sort options in some cases need as const, type casts or type guards
• Simplify types for metric aggregations

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 4b29455d991a5401dd0cfbd019930714ddd96e83

[sorenlouv]

How does this work? Can it pass the options, and if restTotalHitsAsInt is true it will type the response accordingly? That's pretty cool 👍

One thing: Mostly restTotalHitsAsInt is written as rest_total_hits_as_int so maybe we should require it as such?

[sorenlouv]

restTotalHitsAsInt is only used by Lens:

rest_total_hits_as_int is used 54 places

[dgieselaar]

I figured it would look kind of weird if we add more options. If we add something, for some reason, that does not have a query parameter equivalent, do we define it in snake_case then as well?

[wylieconlon]

The bigger issue here is not snake case, but it's that this type is really confusing in practice. I tried getting the elasticsearch-js client to improve typings in the way you're proposing here, but there are some strong caveats.

Here is a discussion I had with @delvedor: elastic/elasticsearch-js#970

Specifically, I've extended the playground example here to demonstrate why this is is so tricky

The biggest caveat here is that Response<{ rest_total_hits_as_int: true}> is not the same as

const request = { rest_total_hits_as_int: true };
Response<typeof request>;

For example, this type is totally valid, which I found unintuitive:

const f = { result: true };
const r: typeof f['result'] = false;

[dgieselaar]

I think TS automatically widens boolean and string literals to respectively booleans and strings. If you change that to { result: true as const }, it will show a TS error. You can replicate that in your playground example as well.

A type definition (rather than a type inferred from a value) does not have the same issue. true remains true, rather than being widened to boolean.

[wylieconlon]

Right, that's what I meant by "confusing in practice." That being said, this is optional and has the correct defaults. So I think this is fine.

[sorenlouv]

This looks really good. Great work!

[sorenlouv]

Why is this change necessary? Doesn't idx always return T | undefined?

[sorenlouv]

Oh, terms bucket key type now accounts for the fact it can be both number and string. That's a little annoying we have to do this everywhere...

Why is this not necessary for serviceName: bucket.key ?

[dgieselaar]

Agree that it's a little annoying, but we could easily run into it ourselves, if we start aggregating on status code for instance. I thought this was an issue for the Lens plugin as well. I figured it would be best to just bite the bullet and have more correct types.

I think we're not using it in a way where it is necessary for it to be a string. Looks like it's used in a ManagedTable, which doesn't really care what type you pass to it. We're using it as a render property, but we have to explicitly type that anyway, so the issue never surfaces. I think it would be best to add a as string there as well.

[wylieconlon]

Yes, we also ran into this issue. We want to start making it easier to build Terms aggregations on dates.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 9efe673a6d20b00ea5d8428f936a45e195386630

[dgieselaar]

@wylieconlon given that you've worked with these types before, I've added you as a reviewer, I can imagine you would have some useful feedback.

[wylieconlon]

I really like these type definitions, but it's clear to me that they are broadly useful within Kibana- not just for APM or Lens, but also for any other use. I've previously talked with the Clients team and @delvedor about whether they have any appetite to help us maintain these, and the answer was that they could not guarantee their correctness over time. But instead of saying "these types are specific to APM" I feel pretty strongly that these should be maintained as helpful types that anyone developing typescript in Kibana can use.

I do like the core of this change, decoupling this from the elasticsearch types. This will make it easier to move all of this code into a top-level directory.

[wylieconlon]

For extended_bounds I'm not sure that both min and max are required- all the examples seem to set both, but is that really true in practice?

[dgieselaar]

You're right, neither of them are required, and they can be a string to. Will fix.

[wylieconlon]

Suggested change

	precision_treshold?: number;
	precision_threshold?: number;

[wylieconlon]

As long as we're adding such strong request types, it bothers me that the types for filters here are any. You've already defined the ESFilter type below, and there is probably an extension for NamedEsFilter that can be used here

[dgieselaar]

I think it's mostly because I don't really trust the ESFilter type. We often have to explicitly type it (rather than TypeScript being able to infer it from the value), and the type of filters is so complicated that it seems like an entirely separate project (which I might give a shot at some point).

[wylieconlon]

Is there a reason you're not using the hits type?

[dgieselaar]

Just did not consider it, that's a great suggestion actually. I'll give it a shot.

[wylieconlon]

Thanks for adding this more accurate filters type!

[wylieconlon]

The bigger issue here is not snake case, but it's that this type is really confusing in practice. I tried getting the elasticsearch-js client to improve typings in the way you're proposing here, but there are some strong caveats.

Here is a discussion I had with @delvedor: elastic/elasticsearch-js#970

Specifically, I've extended the playground example here to demonstrate why this is is so tricky

The biggest caveat here is that Response<{ rest_total_hits_as_int: true}> is not the same as

const request = { rest_total_hits_as_int: true };
Response<typeof request>;

For example, this type is totally valid, which I found unintuitive:

const f = { result: true };
const r: typeof f['result'] = false;

[dgieselaar]

@wylieconlon Thanks for the feedback, it's awesome. I do think it's worth having a discussion on how to scope these filters - a concern we had that our types would not be correct, or that correct types (that cover many use cases) would mean that the type fidelity would be so low that they would lose much of their value. I'll be off for a few days, but maybe we can discuss somewhere next week (I'm off for a few days) on how we can make this usable for others in Kibana. I'm pretty excited for others to start using this 👍

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 084af75d8824b6e7ce1483cdf8adf351e38363f4

[wylieconlon]

This is definitely the right direction, so I'm going to approve with the understanding that we will look for a directory where these type definitions can live outside of any specific plugin.

[wylieconlon]

Yes, we also ran into this issue. We want to start making it easier to build Terms aggregations on dates.

[wylieconlon]

Right, that's what I meant by "confusing in practice." That being said, this is optional and has the correct defaults. So I think this is fine.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 7d0765e