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.

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] Consolidate various throughput calculations to a utility function #89486

Closed
ogupte opened this issue Jan 27, 2021 · 5 comments · Fixed by #89578
Closed

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

ogupte opened this issue Jan 27, 2021 · 5 comments · Fixed by #89578
Assignees
Labels
Team:APM All issues that need APM UI Team support technical debt Improvement of the software architecture and operational architecture v7.12.0

Comments

@ogupte
Copy link
Contributor

ogupte commented Jan 27, 2021

We have many instances of const deltaAsMinutes = (end - start) / 1000 / 60; followed by value / deltaAsMinutes accross APM UI, these should all be using a shared function which defines this logic once.

These instances can be found in x-pack/plugins/apm/server/lib/*:

  • observability_overview/get_transaction_coordinates.ts
  • services/get_service_dependencies/index.ts
  • services/get_service_instances/get_service_instance_transaction_stats.ts
  • services/get_service_transaction_groups/get_transaction_groups_for_page.ts
  • services/get_service_transaction_groups/merge_transaction_group_data.ts
  • services/get_services/get_service_transaction_stats.ts
  • services/get_throughput.ts
  • transactions/get_throughput_charts/index.ts
  • transactions/get_throughput_charts/transform.ts

Related: #89350

@ogupte ogupte added [zube]: In Progress Team:APM All issues that need APM UI Team support technical debt Improvement of the software architecture and operational architecture labels Jan 27, 2021
@ogupte ogupte self-assigned this Jan 27, 2021
@elasticmachine
Copy link
Contributor

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

@dgieselaar
Copy link
Member

There's also the ES rate aggregation. Previously that was not a great option because it didn't support histograms but now that we can use _doc_count it might become a viable alternative again

@sorenlouv
Copy link
Member

The rate agg sounds great! Would love to outsource this to ES. Less code for us and seems more solid.
This would only be for timeseries data and not for the overall averages, right?

@nreese nreese added v7.12.0 and removed v7.12 labels Jan 27, 2021
@dgieselaar
Copy link
Member

@sqren yes, i assumed this issue was also about rate per date histo bucket but not sure. Rate only is allowed as a child of a date histo agg, but maybe it can work outside of one as well, but the semantics are less clear. E.g. it would need a fixed start/end or it could take the min/max of a field as the range. Not sure if it's worth it though

@sorenlouv
Copy link
Member

Rate only is allowed as a child of a date histo agg, but maybe it can work outside of one as well, but the semantics are less clear. E.g. it would need a fixed start/end or it could take the min/max of a field as the range. Not sure if it's worth it though

Yes, that's what I understood as well. Probably better to only use it for timeseries and keep calculating the averages manually like today.

On a related note I wonder if of this will be useful in tackling: #89497

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:APM All issues that need APM UI Team support technical debt Improvement of the software architecture and operational architecture v7.12.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants