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.

Sign up for GitHub

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

Jump to bottom

[APM] Add table tabs showing summary of metrics #153044

Merged
merged 37 commits into from
Apr 25, 2023
Merged

[APM] Add table tabs showing summary of metrics #153044

merged 37 commits into from
Apr 25, 2023

Conversation

MiriamAparicio
Copy link
Contributor

@MiriamAparicio MiriamAparicio commented Mar 9, 2023 •
edited
Loading

Closes #146877

Notes:

  • Crash rate is calculated per sessions, the same session ID is kept after a crash, so a session can have more than one crash.
  • App number of launches is not available
  • Error rate stays out for now
  • Http requests, host.os.version and service.version is only available on transactions, metrics and errors. Not spans for now, there are two issues opened to fix this for the apm mobile agents team
  • Instead of the View Load (not available), we show Throughput
  • The filters (+ -) will be added in a follow-up PR

Pending:

  • API tests
  • e2e tests
Screen.Recording.2023-04-25.at.12.50.56.mov

@MiriamAparicio MiriamAparicio force-pushed the 146877-mobile-tables-metrics branch from 21305a1 to fcc8c82 Compare March 21, 2023 16:02
@MiriamAparicio MiriamAparicio added Team:APM All issues that need APM UI Team support release_note:feature Makes this part of the condensed release notes apm:mobile v8.8.0 ci:cloud-deploy Create or update a Cloud deployment labels Mar 21, 2023
@MiriamAparicio MiriamAparicio marked this pull request as ready for review March 21, 2023 17:20
@MiriamAparicio MiriamAparicio requested a review from a team as a code owner March 21, 2023 17:20
@elasticmachine
Copy link
Contributor

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

yngrdyn
yngrdyn reviewed Mar 22, 2023
View reviewed changes
x-pack/plugins/apm/public/components/app/mobile/transaction_overview/index.tsx Show resolved Hide resolved
...lic/components/app/mobile/transaction_overview/transaction_overview_tabs/app_version_tab.tsx Outdated Show resolved Hide resolved
.../public/components/app/mobile/transaction_overview/transaction_overview_tabs/devices_tab.tsx Outdated Show resolved Hide resolved
...ns/apm/public/components/app/mobile/transaction_overview/transaction_overview_tabs/index.tsx Outdated Show resolved Hide resolved
...blic/components/app/mobile/transaction_overview/transaction_overview_tabs/os_version_tab.tsx Outdated Show resolved Hide resolved
...ic/components/app/mobile/transaction_overview/transaction_overview_tabs/transactions_tab.tsx Outdated Show resolved Hide resolved
...s/app/mobile/transaction_overview/transaction_overview_tabs/use_mobile_statistics_fetcher.ts Outdated Show resolved Hide resolved
...s/app/mobile/transaction_overview/transaction_overview_tabs/use_mobile_statistics_fetcher.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/components/shared/transactions_table/index.tsx Outdated Show resolved Hide resolved
@MiriamAparicio MiriamAparicio force-pushed the 146877-mobile-tables-metrics branch from 829708a to 288a836 Compare March 22, 2023 13:00
dgieselaar
dgieselaar reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

Can you add API tests?

@MiriamAparicio
Copy link
Contributor Author

/oblt-deploy

@MiriamAparicio MiriamAparicio removed the ci:cloud-deploy Create or update a Cloud deployment label Mar 22, 2023
@MiriamAparicio
Copy link
Contributor Author

MiriamAparicio commented Mar 23, 2023 •
edited
Loading

Can you add API tests?

I'm working on getting the data we need in sythtrace to be able to test

@MiriamAparicio MiriamAparicio force-pushed the 146877-mobile-tables-metrics branch from b0b9dc7 to b3121cc Compare March 24, 2023 18:09
dgieselaar
dgieselaar reviewed Apr 13, 2023
View reviewed changes
x-pack/plugins/apm/server/routes/mobile/get_mobile_detailed_statistics_by_field.ts Outdated
`get_mobile_detailed_statistics_by_${field}`,
{
apm: {
events: [ProcessorEvent.transaction],
Copy link
Member

Choose a reason for hiding this comment

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

do we need to always query transaction events? are there scenarios where we can use transaction metrics, maybe even rolled up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mobile fields (device, etc) are not added into metrics due to dimension explosion so for now, we rely on raw span and transaction documents for the mobile data

@MiriamAparicio MiriamAparicio force-pushed the 146877-mobile-tables-metrics branch from 043cd19 to d07d024 Compare April 19, 2023 10:41
@MiriamAparicio MiriamAparicio force-pushed the 146877-mobile-tables-metrics branch from c0be4d4 to 578b81e Compare April 19, 2023 13:39
@MiriamAparicio MiriamAparicio mentioned this pull request Apr 24, 2023
Open
dgieselaar
dgieselaar reviewed Apr 25, 2023
View reviewed changes
packages/kbn-apm-synthtrace/src/scenarios/mobile.ts Show resolved Hide resolved
...ponents/app/mobile/transaction_overview/transaction_overview_tabs/stats_list/get_columns.tsx Outdated
{
field: 'name',
name: i18n.translate(
'xpack.apm.mobile.transactions.overview.table.nameLabel',
Copy link
Member

Choose a reason for hiding this comment

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

why is this nameLabel and the other one are ...ColumnLabel? Would be good to keep this consistent. Same applies to crash rate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed here a79ebaa

...ponents/app/mobile/transaction_overview/transaction_overview_tabs/stats_list/get_columns.tsx
}
),
align: RIGHT_ALIGNMENT,
render: (_, { crashRate }) => {
Copy link
Member

Choose a reason for hiding this comment

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

I realize this is out of scope, but why don't we have a spark plot/comparison for crash rate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the designs, but I will take a note and ask @boriskirov when he is back

...ic/components/app/mobile/transaction_overview/transaction_overview_tabs/stats_list/index.tsx Outdated
Comment on lines 52 to 59
isLoading
? i18n.translate('xpack.apm.statsTable.loading', {
defaultMessage: 'Loading...',
})
: i18n.translate('xpack.apm.statsTable.noDataLabel', {
defaultMessage: 'No data found',
})
}
Copy link
Member

Choose a reason for hiding this comment

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

statsTable seems broad, and don't we have labels or a component that we can reuse to wrap it in a loading state? I thought we did but maybe we removed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could find a component for that, I checked how we did for other tables
I will improve the i18n label

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed here a79ebaa

x-pack/plugins/apm/server/routes/mobile/get_mobile_detailed_statistics_by_field.ts Outdated
Comment on lines 77 to 79
apm: {
events: [ProcessorEvent.transaction],
},
Copy link
Member

Choose a reason for hiding this comment

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

@MiriamAparicio this feedback is still important to address IMHO.

x-pack/plugins/apm/server/routes/mobile/get_mobile_main_statistics_by_field.ts Outdated
Comment on lines 52 to 59
end,
field,
}: Props) {
const response = await apmEventClient.search(
`get_mobile_transaction_events_main_statistics_by_field`,
{
apm: {
events: [ProcessorEvent.transaction, ProcessorEvent.error],
Copy link
Member

Choose a reason for hiding this comment

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

Can you use sources here as well? You'll need to add an ApmDocumentType for errors.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this should actually be two requests, one for transaction events for latency and sessions, and then a separate one for errors to get the number of crashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed here a79ebaa

x-pack/plugins/apm/server/routes/mobile/get_mobile_main_statistics_by_field.ts Outdated
],
},
},
_source: [AGENT_NAME],
Copy link
Member

Choose a reason for hiding this comment

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

What's the point if _source if size is set to 0?

Copy link
Member

Choose a reason for hiding this comment

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

I guess you're not using it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch was a leftover

x-pack/test/apm_api_integration/tests/mobile/mobile_detailed_statistics_by_field.spec.ts
field: 'service.version',
});
expect(isEmpty(response.currentPeriod)).to.be.equal(false);
expect(isEmpty(response.previousPeriod)).to.be.equal(true);
Copy link
Member

Choose a reason for hiding this comment

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

This really needs value-specific tests, not just whether it's empty or not. This is important to make sure we calculate the statistics correctly.

Copy link
Member

Choose a reason for hiding this comment

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

100% agree! In general we are a bit light on the value-specific tests (not just this PR).
Afair back when we used ES archives heavily, the recommendation was to NOT use value-specific tests, because it was time consuming to update them when the archive changed.
That is no longer the case, and perhaps we need to broadcast that.

A tech session on "API test best practices" would be great.

x-pack/test/apm_api_integration/tests/mobile/mobile_main_statistics_by_field.spec.ts Outdated
Comment on lines 92 to 94
expect(throughputValues).to.be.eql([
1.2000013333348147, 0.40000044444493826, 0.40000044444493826,
]);
Copy link
Member

Choose a reason for hiding this comment

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

This is more like snapshot testing. How do you calculate these values based on the inputs for generating data? I'd like to see that calculation in this file instead of writing out numbers that could as well be random to the reader.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1322 1330 +8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/apm-synthtrace-client 153 159 +6

Async chunks

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

id before after diff
apm 3.5MB 3.5MB +6.1KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
apm 110 112 +2
Unknown metric groups

API count

id before after diff
@kbn/apm-synthtrace-client 153 159 +6

ESLint disabled line counts

id before after diff
apm 72 73 +1
enterpriseSearch 17 19 +2
securitySolution 396 399 +3
total +6

Total ESLint disabled count

id before after diff
apm 83 84 +1
enterpriseSearch 18 20 +2
securitySolution 476 479 +3
total +6

History

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

@MiriamAparicio MiriamAparicio merged commit b6a91f3 into elastic:main Apr 25, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 25, 2023
@MiriamAparicio MiriamAparicio deleted the 146877-mobile-tables-metrics branch April 25, 2023 14:43
@gbamparop gbamparop added the apm:test-plan-8.8.0 APM UI Test Plan for v8.8.0 label Apr 25, 2023
@MiriamAparicio MiriamAparicio removed the apm:test-plan-8.8.0 APM UI Test Plan for v8.8.0 label Apr 28, 2023
@MiriamAparicio MiriamAparicio mentioned this pull request May 3, 2023
Merged
MiriamAparicio added a commit that referenced this pull request May 16, 2023
@MiriamAparicio
Improve mobile API tests (#156578)
b35265b 
There were some comments regarding the api test done for the mobile
transactions main and detailed statistics as part of this
[PR](#153044)

### Summary
- add value specific test not just check if the response is empty
(detailed statistics timeseries test)
- calculate values based on the inputs for generating data, instead of
like snapshot testing (throughput)

This PR also contains the fixes for the following failing skipped tests 
- #156207
- #156218
jasonrhodes pushed a commit that referenced this pull request May 17, 2023
@MiriamAparicio @jasonrhodes
Improve mobile API tests (#156578)
a3bb8f1 
There were some comments regarding the api test done for the mobile
transactions main and detailed statistics as part of this
[PR](#153044)

### Summary
- add value specific test not just check if the response is empty
(detailed statistics timeseries test)
- calculate values based on the inputs for generating data, instead of
like snapshot testing (throughput)

This PR also contains the fixes for the following failing skipped tests 
- #156207
- #156218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yngrdyn yngrdyn yngrdyn left review comments

@dgieselaar dgieselaar dgieselaar left review comments

@kpatticha kpatticha kpatticha approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees
No one assigned
Labels
apm:mobile backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Team:APM All issues that need APM UI Team support v8.8.0
Projects
None yet
Milestone
No milestone
9 participants
@MiriamAparicio @elasticmachine @kibana-ci @sorenlouv @dgieselaar @yngrdyn @kpatticha @gbamparop @kibanamachine