[APM] Transactions breakdown graph: Add breakdown KPI component#38658
[APM] Transactions breakdown graph: Add breakdown KPI component#38658dgieselaar merged 11 commits intoelastic:masterfrom
Conversation
💔 Build Failed |
aa3673b to
96fcdc1
Compare
💔 Build Failed |
96fcdc1 to
936ddeb
Compare
💚 Build Succeeded |
936ddeb to
cfa8f96
Compare
💔 Build Failed |
cfa8f96 to
562243d
Compare
💔 Build Failed |
formgeist
left a comment
There was a problem hiding this comment.
@dgieselaar Just made some minor design tweak suggestions to the KPI display.
There was a problem hiding this comment.
You could replace this styled component with <EuiIcon type="dot" color={color} />
There was a problem hiding this comment.
I reckon we need to add a min-width to this like: <EuiFlexItem style={{ minWidth: 110 }}>
There was a problem hiding this comment.
I think it looks a little nicer if you add grow={false} to this item; `
|
@dgieselaar Remember, we should add this component, as well as the graph, to the Transaction group detail page as well.
|
562243d to
1fdd265
Compare
💚 Build Succeeded |
|
edit: see opening post for instructions. |
|
@formgeist any suggestions for empty/loading/error states? |
|
@dgieselaar Let me sketch some up 👍 |
|
@dgieselaar I've added an example of the empty state to the original design issue, see the description. Wrt the loading state, I believe we generally have a horizontal loader and show the panels' empty states until the panels and data is available. This is obviously less than ideal if the loading is longer than a few hundred ms but I think we need to solve that in another larger context. I'd prefer showing the panel with its title and label cleanly until data is available keeping a minimum height. If possible wait with displaying the error message until no data is returned for the request. Thoughts? |
💚 Build Succeeded |
1416ba0 to
4d32543
Compare
💔 Build Failed |
4d32543 to
2483bfe
Compare
💔 Build Failed |
2483bfe to
551d640
Compare
💚 Build Succeeded |
|
@formgeist any advice on what kind of error message should be shown (and where)? In the message bar (not sure what it's called) or inline in the component? @ogupte are there any other examples of the UI handling network errors? |
|
@dgieselaar Did you see the empty state example that I made last week? Is this what you're looking for?
|
3cb9d0e to
028a3ad
Compare
💔 Build Failed |
|
retest |
| </h3> | ||
| </EuiTitle> | ||
| </EuiFlexItem> | ||
| {!hideShowChartButton && ( |
There was a problem hiding this comment.
We may want to use the ternary pattern in JSX composition just to be consistent:
{!hideShowChartButton ? (...) : null}There was a problem hiding this comment.
i think it's ok to use flatten. If we find that it really inflates the size of the kibana builds, then we can always optimize it as needed.
💔 Build Failed |
|
retest |
💔 Build Failed |
|
@formgeist I'm going to merge this if you don't mind, it will allow me to prepare the charts PR. Want to keep things moving a bit because of the feature freeze on Tuesday. If there's anything you'd like to improve we can pick it up in #39531. |
|
retest |
💚 Build Succeeded |
Will address any further feedback in #39531.
|
@dgieselaar Yeah, good call on merging this in 👍 |
…tic#38658) * [APM] Transactions breakdown graph: Add breakdown KPI component Closes elastic#36445. * Remove percentage, line wrapping, correct query * Add transaction breakdown chart to transaction details page * Move files into new plugin dir * Add empty state; update labels * Use new mapping * Move urlParams to useTransactionBreakdown hook * Fix types * Only show empty state in TransactionBreakdown if previously data was rendered * Restrict no of KPIs to 20 * Use ternary in render of TransactionBreakdownHeader for consistency
| const { data, error, status } = useFetcher( | ||
| async () => { | ||
| if (serviceName && start && end) { | ||
| return callApi<TransactionBreakdownAPIResponse>({ |
There was a problem hiding this comment.
We have a convention of keeping rest calls separately in services/rest. Any reason to not do that here?
There was a problem hiding this comment.
I think we previously discussed this shortly, whether this abstraction (of placing rest calls in a separate file) was worth it. I opted to give it a shot without one. Will we use the call without a hook? If we do use a separate file, do we need a separate file with the hook as well?
|
|
||
| const hasHits = data && data.length > 0; | ||
|
|
||
| return receivedDataDuringLifetime ? ( |
There was a problem hiding this comment.
Bailing early would make this a bit easier to digest:
if (!receivedDataDuringLifetime) {
return null;
}(that would also be a small optimization if you moved it to the top although my primary motivation is readability)
Closes #36445. (See linked ticket for user story.)
(Based off of #37360, that needs to be merged first for the diff to be accurate).To test this, start the apm-integration-testing server with the following command:
./scripts/compose.py start master --with-opbeans-java --opbeans-java-agent-branch=pr/588/head --apm-server-build https://github.com/elastic/apm-server.git@masterand delete the template for the metric index.Definition of Done for APM UI