[APM] Make aggregations optional in ES response type definition #43451
Conversation
dgieselaar
added
the
release_note:skip
|
|
||
| return { | ||
| title: chartBase.title, | ||
| key: chartBase.key, | ||
| yUnit: chartBase.yUnit, | ||
| totalHits: hits.total, | ||
| series: Object.keys(chartBase.series).map((seriesKey, i) => { | ||
| const agg = aggregations[seriesKey]; | ||
| const agg = aggregations ? aggregations[seriesKey] : { value: null }; |
There was a problem hiding this comment.
Can you do this instead?
| const agg = aggregations ? aggregations[seriesKey] : { value: null }; | |
| const overallValue = idx(aggregations, _=> _[seriesKey].value); |
There was a problem hiding this comment.
Hmm, it's not the same though. It creates a union type of { value: number | null} | undefined which leads to errors further down the line. We would have to change line 68 to:
overallValue: agg ? agg.value : null,still worth it?
There was a problem hiding this comment.
Oh, sorry, misread your suggestion!
There was a problem hiding this comment.
I think you need to adjust ByteFormatter in formatters to also accept undefined. asBytes and asDynamicBytes already accepts number | undefined | null so makes sense for all of the byte formatting functions to behave consistently.
There was a problem hiding this comment.
... aka so doing
overallValue: idx(aggregations, _=> _[seriesKey].value)should still be possible.
| 0, | ||
| MAX_KPIS | ||
| ) | ||
| : []; |
There was a problem hiding this comment.
Can you avoid the ternary like this?
const visibleKpis = sortByOrder(
formatBucket(resp.aggregations || []),
'percentage',
'desc'
).slice(0, MAX_KPIS)There was a problem hiding this comment.
resp.aggregations is not an array, but a plain object. If I pass in a plain object, TypeScript will complain about a type mismatch.
There was a problem hiding this comment.
Ahh, yeah. You could also change formatBucket like:
const formatBucket = (
aggs:
| typeof resp['aggregations']
| Required<typeof resp>['aggregations']['by_date']['buckets'][0]
) => {
if (!aggs) {
return [];
}But if you prefer the ternary that's also totally fine.
x-pack/legacy/plugins/apm/server/lib/transactions/breakdown/index.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.ts
Outdated
Show resolved
Hide resolved
💔 Build Failed |
dgieselaar
force-pushed
the
aggregations-optional
branch
from
abdf885
to
9912b62
Compare
💔 Build Failed |
|
Interestingly enough some of the API tests that run requests without matching indices already expected a 400 (which is actually a 500). All expect a 200 now. |
💔 Build Failed |
| } | ||
| return cb(val); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
I like how you've extracted the logic instead of having it in every formatter 👍
Can we do something similar with the time formatters?
btw. Looks like a similar null check in asDynamicBytes can now be removed.
There was a problem hiding this comment.
Fwiw, this is now fixed
💔 Build Failed |
Closes elastic#43436. Our type definition for ElasticSearch aggregations suggests that the aggregations key will always be defined if we request aggregations. However, that is not the case, as there is an edge case where if no matching indices were found, the aggregations key will be undefined.
dgieselaar
force-pushed
the
aggregations-optional
branch
from
6366abf
to
e986375
Compare
💚 Build Succeeded |
…tic#43451) * [APM] Make aggregations optional in ES response type definition Closes elastic#43436. Our type definition for ElasticSearch aggregations suggests that the aggregations key will always be defined if we request aggregations. However, that is not the case, as there is an edge case where if no matching indices were found, the aggregations key will be undefined. * Review feedback * Support undefined values in number formatters * Expect 200s for all API requests even without matching indices * Bail early for asDynamicBytes as well
* [APM] Make aggregations optional in ES response type definition Closes #43436. Our type definition for ElasticSearch aggregations suggests that the aggregations key will always be defined if we request aggregations. However, that is not the case, as there is an edge case where if no matching indices were found, the aggregations key will be undefined. * Review feedback * Support undefined values in number formatters * Expect 200s for all API requests even without matching indices * Bail early for asDynamicBytes as well
Closes #43436.
Our type definition for ElasticSearch aggregations suggests that the aggregations key will always be defined if we request aggregations. However, that is not the case, as there is an edge case where if no matching indices were found, the aggregations key will be undefined.