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] Service inventory: detailed stats fetched for all services #134844
[APM] Service inventory: detailed stats fetched for all services #134844
Conversation
8e14e35
to
195c880
Compare
...k/plugins/apm/public/components/app/service_overview/service_overview_errors_table/index.tsx
Show resolved
Hide resolved
Pinging @elastic/apm-ui (Team:apm) |
@elasticmachine merge upstream |
…m:cauemarcondes/kibana into apm-service-inventory-fetch-details-stats
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
cy.get('.euiPagination__list').children().should('have.length', 5); | ||
cy.wait('@errorsDetailedStatistics').then((payload) => { | ||
expect(payload.request.body.groupIds).eql( | ||
JSON.stringify([ | ||
'0000000000000000000000000Error 0', | ||
'0000000000000000000000000Error 1', | ||
'0000000000000000000000000Error 2', | ||
'0000000000000000000000000Error 3', | ||
'0000000000000000000000000Error 4', | ||
'0000000000000000000000000Error 5', | ||
'0000000000000000000000000Error 6', | ||
'0000000000000000000000000Error 7', | ||
'0000000000000000000000000Error 8', | ||
'0000000000000000000000000Error 9', | ||
]) | ||
); | ||
}); |
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.
This might be technically correct but it's not super clear what this verifies. The test says "calls detailed API with visible items only".
There are 5 visible items afaict on line 140. Yet, detailed statistics requests 10 values. Isn't that a disconnect? Should those two be the same?
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.
No, there are 5 pages, with 10 items on each of them.
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.
Okay I missed that. But that's also my point: it's not clear what this is verifying (at least not to me). Why are we asserting the number of pages in the pagination? Is that important when ensuring that it only makes requests for the visible items? Perhaps it's useful to assert that there are more pages than the current one.
Most importantly, I think you should make the connection between pageSize
and groupIds
more obvious:
const pageSize = 10;
cy.visit(`${javaServiceErrorsPageHref}&pageSize=${pageSize}`);
cy.wait('@errorsMainStatistics');
cy.get('.euiPagination__list').children().should('be.gt', 1);
cy.wait('@errorsDetailedStatistics').then((payload) => {
expect(payload.request.body.groupIds.length).eql(pageSize)
);
});
expect(payload.request.body.groupIds).eql( | ||
JSON.stringify([ | ||
'000000000000000000000000Error 10', | ||
'000000000000000000000000Error 11', | ||
'000000000000000000000000Error 12', | ||
'000000000000000000000000Error 13', | ||
'000000000000000000000000Error 14', | ||
'000000000000000000000000Error 15', | ||
'000000000000000000000000Error 16', | ||
'000000000000000000000000Error 17', | ||
'000000000000000000000000Error 18', | ||
'000000000000000000000000Error 19', | ||
]) |
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.
Same as above. I'd expect this to be something like:
expect(payload.request.body.groupIds).eql( | |
JSON.stringify([ | |
'000000000000000000000000Error 10', | |
'000000000000000000000000Error 11', | |
'000000000000000000000000Error 12', | |
'000000000000000000000000Error 13', | |
'000000000000000000000000Error 14', | |
'000000000000000000000000Error 15', | |
'000000000000000000000000Error 16', | |
'000000000000000000000000Error 17', | |
'000000000000000000000000Error 18', | |
'000000000000000000000000Error 19', | |
]) | |
expect(payload.request.body.groupIds.length).eql(NUMBER_OF_VISIBLE_ERROR_GROUPS) |
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.
check the comment above.
x-pack/plugins/apm/ftr_e2e/cypress/integration/read_only_user/errors/generate_data.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/ftr_e2e/cypress/integration/read_only_user/errors/generate_data.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/error_group_overview/index.tsx
Outdated
Show resolved
Hide resolved
offset: | ||
comparisonEnabled && isTimeComparison(offset) | ||
? offset | ||
: undefined, | ||
}, | ||
body: { | ||
groupIds: JSON.stringify( | ||
items.map(({ groupId: groupId }) => groupId).sort() |
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.
It looks like items
only contains the current page's group id's. That's fine but different from how this is handled in error_group_overview/index.tsx
. Can we handle this more consistently?
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.
That's why error_group_overview
uses ManagedTable
and service_overview_errors_table
uses EuiBasicTable
. I personally don't like the ManagedTable
since it introduces a bug if it is used more than one time on the same page since it updates the table options in the URL (pageIndex, pageSize, sortField and sortDirection).
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.
There's an open issue to delete the ManagedTable #111328
💚 Build SucceededMetrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
closes #133697
Service Inventory
service.inventory.mov
Errors Overview
Before:
After: