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] Show transaction rate per minute on Observability Overview page #70336

Merged
merged 6 commits into from
Jul 2, 2020
Merged

[APM] Show transaction rate per minute on Observability Overview page #70336

merged 6 commits into from
Jul 2, 2020

Conversation

cauemarcondes
Copy link
Contributor

closes #70332

Returns the transaction rate per minute in a given time range instead of transaction count.

@cauemarcondes cauemarcondes added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 30, 2020
@cauemarcondes cauemarcondes requested a review from a team as a code owner June 30, 2020 15:38
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 30, 2020
@elasticmachine
Copy link
Contributor

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

smith
smith approved these changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

This looks ok. How do I view the dashboard in the UI at this time?

x-pack/plugins/apm/public/services/rest/observability_dashboard.ts Outdated
@@ -47,7 +47,8 @@ export const fetchLandingPageData = async (
'xpack.apm.observabilityDashboard.stats.transactions',
{ defaultMessage: 'Transactions' }
),
value: sum(transactionCoordinates.map((coordinates) => coordinates.y)),
value:
mean(transactionCoordinates.map((coordinates) => coordinates.y)) || 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Beware that mean can return NaN if any of the coordinates.y values here are non-numeric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up @smith , I added a condition to check if the array is not empty and to return 0 in case corrdinate.y is undefined. ffedc7f#diff-cf9933de7395c67ff230849c1c748111R50-R54

Copy link
Contributor

Choose a reason for hiding this comment

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

That will return 0 if any y value is non-numeric, which is fine if that's the behavior you want. When I ran into this I did values.map(value => value.y).filter(y => isFinite(y)).

(I can't remember if the data I was looking at had null or NaN or undefined, but let's say null for now) The first method makes it 0 if any null are found and the second gives you the mean of the the non-null values. I suppose you could also do values.map(value => value.y ?? 0), which would convert all the null values to zeroes and give you the mean of that.

sorenlouv
sorenlouv reviewed Jul 1, 2020
View reviewed changes
x-pack/plugins/apm/public/services/rest/observability_dashboard.ts
mean(
transactionCoordinates
.map(({ y }) => y)
.filter((y) => y && isFinite(y))
Copy link
Member

@sorenlouv sorenlouv Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I think you can do:

Suggested change
.filter((y) => y && isFinite(y))
.filter(isFinite)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isFinite requires a number, and y can be number | undefined. That's why I didn't do what you've suggested.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@cauemarcondes cauemarcondes merged commit 6aeda64 into elastic:master Jul 2, 2020
@cauemarcondes cauemarcondes deleted the apm-transaction-per-min branch July 2, 2020 09:01
@cauemarcondes cauemarcondes mentioned this pull request Jul 2, 2020
Merged
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Jul 2, 2020
@cauemarcondes
[APM] Show transaction rate per minute on Observability Overview page (…
49a0de5 
…elastic#70336)

* changing transaction count to transaction rate per second

* sanity check coordinates before calculate the mean

* sanity check coordinates before calculate the mean

* removing extend_bounds to return empty when no data is available
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 2, 2020
@gmmorris
Merge branch 'master' into alerting/built-in-alerts-feature
0e001e6 
* master: (46 commits)
  [Visualize] Add missing advanced settings and custom label for pipeline aggs (elastic#69688)
  Use dynamic: false for config saved object mappings (elastic#70436)
  [Ingest Pipelines] Error messages (elastic#70167)
  [APM] Show transaction rate per minute on Observability Overview page (elastic#70336)
  Filter out error when calculating a label (elastic#69934)
  [Visualizations] Each visType returns its supported triggers (elastic#70177)
  [Telemetry] Report data shippers (elastic#64935)
  Reduce SavedObjects mappings for Application Usage (elastic#70475)
  [Lens] fix dimension label performance issues (elastic#69978)
  Skip failing endgame tests (elastic#70548)
  [SIEM] Reenabling Cypress tests (elastic#70397)
  [SIEM][Security Solution][Endpoint] Endpoint Artifact Manifest Management + Artifact Download and Distribution (elastic#67707)
  [Security] Adds field mapping support to rule creation (elastic#70288)
  SECURITY-ENDPOINT: add fields for events to metadata document (elastic#70491)
  Fixed assertion in hybrid index pattern test to iterate through indices (elastic#70130)
  [SIEM][Exceptions] - Exception builder component (elastic#67013)
  [Ingest Manager] Rename data sources to package configs (elastic#70259)
  skip suites blocking es snapshot promomotion (elastic#70532)
  [Metrics UI] Fix asynchronicity and error handling in Snapshot API (elastic#70503)
  fix export response (elastic#70473)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 6, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 7, 2020
cauemarcondes added a commit that referenced this pull request Jul 7, 2020
@cauemarcondes @elasticmachine @sorenlouv
[7.x] [APM] Show transaction rate per minute on Observability Overvie…
f56a6c3 
…w page (#70336) (#70566)

* [APM] Show transaction rate per minute on Observability Overview page (#70336)

* changing transaction count to transaction rate per second

* sanity check coordinates before calculate the mean

* sanity check coordinates before calculate the mean

* removing extend_bounds to return empty when no data is available

* Fix lodash

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Søren Louv-Jansen <sorenlouv@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smith smith smith approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Show transaction rate per minute on Observability Overview page
5 participants
@cauemarcondes @elasticmachine @kibanamachine @smith @sorenlouv