Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[APM] Consolidate various throughput calculations to a utility function #89578

Merged
merged 3 commits into from
Feb 1, 2021
Merged

[APM] Consolidate various throughput calculations to a utility function #89578

merged 3 commits into from
Feb 1, 2021

Conversation

ogupte
Copy link
Contributor

@ogupte ogupte commented Jan 28, 2021 •
edited
Loading

Closes #89486.

Creates the helper function calculateThroughput and uses it in the various places where we were previously calculating TPM values.

@ogupte ogupte added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Jan 28, 2021
@ogupte ogupte force-pushed the apm-89486-consolidate-throughpout-calc branch from 5daeb7b to 1988d42 Compare January 29, 2021 02:57
@ogupte ogupte force-pushed the apm-89486-consolidate-throughpout-calc branch from 1988d42 to daf9275 Compare January 29, 2021 03:04
@ogupte ogupte marked this pull request as ready for review January 29, 2021 08:39
@ogupte ogupte requested a review from a team as a code owner January 29, 2021 08:39
sorenlouv
sorenlouv reviewed Jan 29, 2021
View reviewed changes
x-pack/plugins/apm/server/lib/services/get_services/get_service_transaction_stats.ts
@@ -179,16 +168,18 @@ export async function getServiceTransactionStats({
),
},
transactionsPerMinute: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are at it: can you rename this to "throughput"?

Copy link
Member

@sorenlouv sorenlouv Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above it says "avgResponseTime". Perhaps you can rename that to "averageLatency" - or simply "latency"

@botelastic botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Jan 29, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@sorenlouv
Copy link
Member

I noticed two instances where we calculate throughout that doesn't use the new helper:

Service Map

kibana/x-pack/plugins/apm/server/lib/service_map/get_service_map_service_node_info.ts

Lines 179 to 182 in 5f8f21b

return {
avgTransactionDuration: response.aggregations?.duration.value ?? null,
avgRequestsPerMinute: totalRequests > 0 ? totalRequests / minutes : null,
};

Transaction Groups

kibana/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts

Line 83 in 5f8f21b

transactionsPerMinute: (item.count ?? 0) / minutes,

Would it be possible to do a search/replace and update "transactionsPerMinute" to "throughput" and "avgRequestsPerMinute" to "avgThroughput" (might be a little out of scope for this PR)

sorenlouv
sorenlouv reviewed Jan 29, 2021
View reviewed changes
x-pack/plugins/apm/server/lib/transactions/get_throughput_charts/transform.ts
}) {
const { start, end } = setupTimeRange;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we could also update the calculation below (on line 31):

y: bucket.count.value / (bucketSize / 60),

Copy link
Member

@sorenlouv sorenlouv Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw. this is a good candidate for using the rate agg that we also have an issue for.

Perhaps doing that could be a follow up to this PR (or something you could do as part of this one?)

sorenlouv
sorenlouv approved these changes Jan 29, 2021
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a few places where we don't use the helper yet. Apart from that it lgtm

@ogupte
Copy link
Contributor Author

ogupte commented Jan 29, 2021

Noticed a few places where we don't use the helper yet. Apart from that it lgtm

I thought i caught them all, so thanks for double checking.

@ogupte
Copy link
Contributor Author

ogupte commented Feb 1, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ogupte ogupte merged commit 07f8e48 into elastic:master Feb 1, 2021
@ogupte ogupte mentioned this pull request Feb 1, 2021
Merged
ogupte added a commit to ogupte/kibana that referenced this pull request Feb 1, 2021
@ogupte
[APM] Consolidate various throughput calculations to a utility functi…
aa1b769 
…on (elastic#89486) (elastic#89578)
ogupte added a commit that referenced this pull request Feb 1, 2021
@ogupte
[APM] Consolidate various throughput calculations to a utility functi…
25b4f2a 
…on (#89486) (#89578) (#89921)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Consolidate various throughput calculations to a utility function
4 participants
@ogupte @elasticmachine @sorenlouv @kibanamachine