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] Display throughput as tps (instead of tpm) when bucket size < 60 seconds #107850

Merged
merged 12 commits into from
Aug 10, 2021

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Aug 7, 2021

Closes #35938

This only converts the units from tpm to tps for the throughput chart, when the bucketsize is less than 1 minute. Throughput metrics (stats) in tables are left untouched.

Before

image

image

After

image

image

@sorenlouv
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@sorenlouv sorenlouv force-pushed the convert-tpm-to-tps-for-charts-only branch from ea1a98f to 43fe126 Compare August 7, 2021 11:28
@sorenlouv sorenlouv marked this pull request as ready for review August 7, 2021 11:41
@sorenlouv sorenlouv requested a review from a team as a code owner August 7, 2021 11:41
@sorenlouv sorenlouv added release_note:fix v7.15.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 7, 2021
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 7, 2021
@elasticmachine
Copy link
Contributor

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

@sorenlouv
Copy link
Member Author

jenkins run the e2e

@sorenlouv sorenlouv changed the title [APM] Convert tpm to tps when throughput < 60 [APM] Convert tpm to tps when bucket size is less than 60 Aug 9, 2021
@sorenlouv sorenlouv changed the title [APM] Convert tpm to tps when bucket size is less than 60 [APM] Convert tpm to tps when bucket size < 60 seconds Aug 9, 2021
@sorenlouv sorenlouv changed the title [APM] Convert tpm to tps when bucket size < 60 seconds [APM] Display throughput as tps (instead of tpm) when bucket size < 60 seconds Aug 9, 2021
})
: i18n.translate('xpack.apm.serviceOverview.tpsHelp', {
defaultMessage:
'Throughput is measured in tps (transactions per second)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could inform the user when we break from tpm to tps?!

Copy link
Member Author

@sorenlouv sorenlouv Aug 9, 2021

Choose a reason for hiding this comment

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

Yes, I thought about it but didn't want it to be too confusing. So rather than explaining what both tpm and tps is, it just explains the current state.

WDYT @formgeist ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need to clarify when we switch. I would suggest only to clearly show a different measurement when we switch to seconds. The default measure is minutes, which we can still show in a tooltip next to the title. But we'll change the title and tooltip when we switch, like so;

throughput

Copy link
Member Author

Choose a reason for hiding this comment

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

@formgeist I added the (per second) label

@cauemarcondes
Copy link
Contributor

@sqren It looks like the chart legends are not correct.

Screen Shot 2021-08-09 at 10 03 09

@sorenlouv sorenlouv requested a review from a team as a code owner August 9, 2021 21:51
@sorenlouv sorenlouv force-pushed the convert-tpm-to-tps-for-charts-only branch from 6275679 to 8f4f227 Compare August 10, 2021 07:25
@sorenlouv
Copy link
Member Author

@cauemarcondes All comments addressed.

@sorenlouv
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@sorenlouv
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 4.3MB 4.3MB +2.5KB
Unknown metric groups

Non-exported public API item count

id before after diff
apm 29 30 +1

History

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

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

LGTM

@sorenlouv sorenlouv merged commit 1c9edeb into elastic:master Aug 10, 2021
@sorenlouv sorenlouv deleted the convert-tpm-to-tps-for-charts-only branch August 10, 2021 21:31
@formgeist
Copy link
Contributor

Found a tiny copy bug and reported it here #108171

jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 11, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (101 commits)
  [ML] APM Latency Correlations: Field/value candidates prioritization (elastic#107370)
  [Reporting] Add lenience to a test on the order of asserted logs (elastic#108135)
  [Lens] fix do not submit invalid query in filtered metric (elastic#107542)
  skip flaky test (elastic#108043)
  fix newly introduced type error (elastic#107593)
  [Reporting] server side code clean up (elastic#106940)
  [build_ts_refs] improve caches, allow building a subset of projects (elastic#107981)
  [APM] Add new ftr_e2e to kibana CI and remove current e2e tests. (elastic#107593)
  add manage rules link to alerts dropdown (elastic#107950)
  [ML] Enable Index data visualizer document count chart to update time range query (elastic#106438)
  [Security Solutions][Detection Engine] Fixes "undefined" crash for author field by adding a migration for it (elastic#107230)
  [Actions UI] Fixed Jira Api token label. (elastic#107776)
  [Alerting UI] Fixed display permissions for edit/delete buttons when user has read only access. (elastic#107996)
  [Maps] fix code owners (elastic#108106)
  Update EMS landing page url (elastic#108102)
  Do not render page header for loading domains (elastic#108078)
  Update dependency @elastic/charts to v33.2.2 (elastic#107939)
  [APM] Display throughput as tps (instead of tpm) when bucket size < 60 seconds (elastic#107850)
  [Fleet] Fix all category count (elastic#108089)
  [Security Solution][Bug] - Disable alert table RBAC until fields sorted (elastic#108034)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/export_types/common/generate_png.ts
#	x-pack/plugins/reporting/server/lib/screenshots/index.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
sorenlouv added a commit to sorenlouv/kibana that referenced this pull request Aug 11, 2021
sorenlouv added a commit that referenced this pull request Aug 11, 2021
…0 seconds (#107850) (#108217)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:fix Team:APM All issues that need APM UI Team support v7.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] "Request per minute" visualization is misleading
6 participants