-
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] Fixes inconsistent labels for throughput & latency #88101
[APM] Fixes inconsistent labels for throughput & latency #88101
Conversation
Pinging @elastic/apm-ui (Team:apm) |
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.
Perhaps the description of the issue wasn't clear enough, but we should align everywhere else to the Service overview labels. I've suggested some changes. Beyond this, I found a few issues, perhaps we can fix these in this PR.
- Rename the "Average duration by span type". Somehow we renamed the panel, but never changed the chart, so the title is actually misleading. We should "revert" back to the original title; "Time spent by span type".
- The Latency visualization is set to "Average" but the label in the table says "95th". Not sure when this was introduced, but it's reproducible on
master
.
cc @sqren
x-pack/plugins/apm/public/components/app/service_inventory/ServiceList/index.tsx
Outdated
Show resolved
Hide resolved
{ | ||
defaultMessage: 'Trans. per minute', | ||
defaultMessage: 'Traffic (per min)', |
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.
defaultMessage: 'Traffic (per min)', | |
defaultMessage: 'Traffic', |
x-pack/plugins/apm/public/components/shared/charts/transaction_charts/index.tsx
Outdated
Show resolved
Hide resolved
@formgeist: Good catch! I've fixed it in #88188 |
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 changes 👍
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
* Closes elastic#87483. Updates several labels to be consistent accross views. * Updates integration tests/snapshots with new term * Changes "traffic" => "throughput" in the labels * addresses feedback from PR * includes tpm unit in Observability overview page and updates test
* Closes elastic#87483. Updates several labels to be consistent accross views. * Updates integration tests/snapshots with new term * Changes "traffic" => "throughput" in the labels * addresses feedback from PR * includes tpm unit in Observability overview page and updates test
@@ -75,7 +75,7 @@ const traceListColumns: Array<ITableColumn<TraceGroup>> = [ | |||
render: (time: number) => asMillisecondDuration(time), | |||
}, | |||
{ | |||
field: 'trafficPerMinute', | |||
field: 'transactionsPerMinute', |
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.
Shouldn't this have been changed to throughput
instead of transactionsPerMinute
?
@@ -41,7 +41,7 @@ describe('ServiceList', () => { | |||
const service: any = { | |||
serviceName: 'opbeans-python', | |||
agentName: 'python', | |||
trafficPerMinute: { | |||
transactionsPerMinute: { |
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.
throughput
)} | ||
</span> | ||
</EuiTitle> | ||
<TimeseriesChart | ||
fetchStatus={throughputChartsStatus} | ||
id="trafficPerMinute" | ||
id="transactionsPerMinute" |
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.
throughput
Closes #87483. Updates several labels to be consistent across views.