-
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] Services inventory: add time comparisons to match service overview design #107094
[APM] Services inventory: add time comparisons to match service overview design #107094
Conversation
Pinging @elastic/apm-ui (Team:apm) |
@elasticmachine merge upstream |
x-pack/plugins/apm/public/components/app/service_inventory/index.tsx
Outdated
Show resolved
Hide resolved
…condes/kibana into apm-service-inventory-comparison
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.
Overall LGTM! 👍 Super excited to get these comparisons into the service inventory as well.
I noticed an "edge-case" where one of the services is not reporting any data (and only has a single data point), I wonder if there's nothing rendered in the chart, we should show the N/A state and the icon in the chart cell?
@elasticmachine merge upstream |
x-pack/plugins/apm/public/components/app/service_inventory/index.tsx
Outdated
Show resolved
Hide resolved
return { servicesData: data, servicesStatus: status }; | ||
return { | ||
servicesData: mainStatistics, | ||
servicesStatus: status, |
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.
Are we only interested in the mainStatistics
status, and not also the comparison status?
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.
Yes, the status from the mainStatistics
is going to be used to show an error message if that was the case. And if any happened while fetching the comparison statistics it just wouldn't show the sparklines.
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.
But the comparison statistics is used to define if the loading spinner should be visible or not. https://github.com/elastic/kibana/pull/107094/files#diff-1f8752bfc4d79520010b5d507b96176f454830da1dc8e2091578f66e42007d02R145-R148
x-pack/plugins/apm/public/components/app/service_inventory/index.tsx
Outdated
Show resolved
Hide resolved
@@ -23,10 +22,6 @@ interface Props { | |||
export function NoServicesMessage({ historicalDataFound, status }: Props) { | |||
const upgradeAssistantHref = useUpgradeAssistantHref(); | |||
|
|||
if (status === 'loading') { | |||
return <LoadingStatePrompt />; | |||
} |
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.
Is this no longer needed?
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.
not really, the loading state is going to be handled by the EuiBasicTable now, https://github.com/elastic/kibana/pull/107094/files#diff-5eecffd3503b82b7d3d75728d267e8e1f3a98f9b2228a0f7a86c848c4d286f83R130
x-pack/plugins/apm/public/components/app/service_inventory/service_list/index.tsx
Show resolved
Hide resolved
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.
lgtm
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
…iew design (elastic#107094) * adding comparison to inventory page * new api to get detailed statistics * show comparison data * adding api test * fixing unit test * fixing ts issue * adding loading to table * refactoring * fixing TS issue * addressing PR comments * fixing merge * addressing PR comments * fixing api test * adding comment Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…iew design (#107094) (#107607) * adding comparison to inventory page * new api to get detailed statistics * show comparison data * adding api test * fixing unit test * fixing ts issue * adding loading to table * refactoring * fixing TS issue * addressing PR comments * fixing merge * addressing PR comments * fixing api test * adding comment Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Cauê Marcondes <[email protected]>
…iew design (elastic#107094) * adding comparison to inventory page * new api to get detailed statistics * show comparison data * adding api test * fixing unit test * fixing ts issue * adding loading to table * refactoring * fixing TS issue * addressing PR comments * fixing merge * addressing PR comments * fixing api test * adding comment Co-authored-by: Kibana Machine <[email protected]>
closes #106318
service.inventory.mov