-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[APM] Remove rate aggregations #114187
[APM] Remove rate aggregations #114187
Conversation
x-pack/test/apm_api_integration/tests/throughput/dependencies_apis.ts
Outdated
Show resolved
Hide resolved
x-pack/test/apm_api_integration/tests/throughput/transactions_metrics_apis.ts
Outdated
Show resolved
Hide resolved
x-pack/test/apm_api_integration/tests/throughput/transactions_metrics_apis.ts
Outdated
Show resolved
Hide resolved
x-pack/test/apm_api_integration/tests/throughput/transactions_metrics_apis.ts
Outdated
Show resolved
Hide resolved
x-pack/test/apm_api_integration/tests/throughput/transactions_metrics_apis.ts
Outdated
Show resolved
Hide resolved
9509b60
to
eb78ad5
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.
Uptime changes LGTM !!
}); | ||
expect(core.http.post).toHaveBeenCalledTimes(1); | ||
expect(core.http.post).toHaveBeenCalledWith('/api/metrics/overview/top', { | ||
body: JSON.stringify({ | ||
sourceId: 'default', | ||
bucketSize, | ||
bucketSize: intervalString, |
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'm having a hard time following why these changes are necessary for the logs and metrics fetchers. Could you please help me understand why we need both bucketSize
and intervalString
and why we switch around their meanings?
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.
On the APM api I need both bucketSize and intervalString now, before we were using bucketSize
as string, which is incorrect, based on the function we use to calculate the bucket size getBucketSize. Now the callback function receives both options, and for you is just a matter of using the intervalString
which will have the same value as the old bucketSize
.
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.
Ok, but as implementers of the a data fetcher it would be good to know what these parameters mean. Are they redundant so we can choose which to use? Probably not, because then we wouldn't need both. But which to use when?
Maybe we could add that info to the parameter type definition?
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.
They are not redundant, although they will have the bucket as string and the bucket as number (seconds) for example if intervalString is 60s
buckeSize will be 60.
But which to use when?
You'll use intervalString
on DateHistogram aggregation, and buckeSize
if you want to calculate the rate of a bucket.
Maybe we could add that info to the parameter type definition?
I'll add a description there.
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.
thanks for the explanation!
const throughputResponse = await callApi({ query: { environment: 'production' } }); | ||
throughput = throughputResponse.body; |
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.
Any reason why callApi
doesn't return body
?
const throughputResponse = await callApi({ query: { environment: 'production' } }); | |
throughput = throughputResponse.body; | |
throughput = await callApi({ query: { environment: 'production' } }); |
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.
yes, I check the status of the request on this test https://github.com/cauemarcondes/kibana/blob/apm-removing-rate-aggs/x-pack/test/apm_api_integration/tests/services/throughput.ts#L65
const previousPeriodMean = meanBy( | ||
throughputResponse.previousPeriod.filter( | ||
(item) => isFiniteNumber(item.y) && item.y > 0 | ||
), | ||
'y' | ||
); |
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.
Is it necessary to filter values out?
const previousPeriodMean = meanBy( | |
throughputResponse.previousPeriod.filter( | |
(item) => isFiniteNumber(item.y) && item.y > 0 | |
), | |
'y' | |
); | |
const previousPeriodMean = meanBy(throughputResponse.previousPeriod, 'y'); |
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.
If I don't filter values out I won't be able to compare it with the RATE variables, monthly because the interval used in the dateHistogram to fetch the comparisons is 1s
and since we generated data every 1m
it means that for every bucket that containes value we'll have 59 others with 0. That's why I though of filtering to only return what has value and get the mean of that.
@elasticmachine merge upstream |
// Size of the bucket in seconds calculated based on a time range. e.g. 60 | ||
bucketSize: number; | ||
// String representing the interval in seconds that is calculated based on a time range. e.g. '60s' | ||
intervalString: string; |
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 description for the two should be identical and just highlight the the type is different:
// Size of the bucket in seconds calculated based on a time range. e.g. 60 | |
bucketSize: number; | |
// String representing the interval in seconds that is calculated based on a time range. e.g. '60s' | |
intervalString: string; | |
// Bucket size in seconds (number) | |
bucketSize: number; | |
// Bucket size in seconds (string) | |
intervalString: string; |
Ideally the two was called the same even:
// Size of the bucket in seconds calculated based on a time range. e.g. 60 | |
bucketSize: number; | |
// String representing the interval in seconds that is calculated based on a time range. e.g. '60s' | |
intervalString: string; | |
// Bucket size in seconds (number) | |
bucketSize: number; | |
// Bucket size in seconds (string) | |
bucketSizeString: string; |
(but that is a larger refactor and probably not worth doing right now)
💚 Build SucceededMetrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
infra
changes LGTM and they seems to be producing the same results
thank you!
* fixing throughput chart api * change backends * adding intervalString to the observability callback functions * fixing transaction group detailed stats * fixing tests * fixing test * fixing obs tests * fixing tests * adding tests * fixing ci * using data generator * changing name * fixing i18n * updating opbs test to use data generator * fixing api tests * fixing tests * using data generator to run the tests * fixing tests * fixing test
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* fixing throughput chart api * change backends * adding intervalString to the observability callback functions * fixing transaction group detailed stats * fixing tests * fixing test * fixing obs tests * fixing tests * adding tests * fixing ci * using data generator * changing name * fixing i18n * updating opbs test to use data generator * fixing api tests * fixing tests * using data generator to run the tests * fixing tests * fixing test Co-authored-by: Cauê Marcondes <[email protected]>
closes #112240