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.

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] Mobile crashes & errors #165892

Merged
merged 82 commits into from
Dec 1, 2023
Merged

[APM] Mobile crashes & errors #165892

merged 82 commits into from
Dec 1, 2023

Conversation

bryce-b
Copy link
Contributor

@bryce-b bryce-b commented Sep 6, 2023

Summary

This PR adds back the Errors tab to mobile apm services under the title Errors & Crashes. This new page is split into too sections: errors, and crashes.

Error Tab:
Screenshot 2023-10-25 at 10 57 00

Crashes Tab:
Screenshot 2023-10-25 at 10 57 35

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

For maintainers

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

I was checking you PR.
What I would do is:

x-pack/plugins/apm/public/components/app/mobile/error_group_overview/tabs/tabs.tsx

  1. would receive an input, this input would be the selected tabId.
  2. conditionally this component will render errors or crashes depending on the selected tabId.

x-pack/plugins/apm/public/components/routing/mobile_service_detail/index.tsx

  1. Modify the params of the route '/mobile-services/{serviceName}/errors' to include our new param tabId (I'd recommend to use a more clear name for example mobileSelectedTab is the one being used already for other screens in mobile)

x-pack/plugins/apm/public/components/app/mobile/error_group_overview/index.tsx

  1. would use useApmParams to get what we have in the url, for this particular case we are interested in tabId
  2. would pass the selectedTab (in this case tabId) to Tabs component

Then you wouldn't need to keep the state in Tabs component converting it in sort of a pure visual component

@cla-checker-service
Copy link

cla-checker-service bot commented Sep 25, 2023

💚 CLA has been signed

@bryce-b bryce-b force-pushed the mobile-errors branch 2 times, most recently from 07c2b39 to 1217563 Compare October 17, 2023 00:00
@bryce-b bryce-b marked this pull request as ready for review October 25, 2023 17:55
@bryce-b bryce-b requested a review from a team as a code owner October 25, 2023 17:55
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Oct 25, 2023
@elasticmachine
Copy link
Contributor

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

@bryce-b bryce-b requested review from a team as code owners October 30, 2023 19:21
Copy link
Contributor

@kpatticha kpatticha left a comment

Choose a reason for hiding this comment

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

I've noticed some code duplication between errors and crashes. We need to remove the unnecessary components

@bryce-b
Copy link
Contributor Author

bryce-b commented Nov 29, 2023

Checks is failing, but I get a "not found" when trying to view the details...

@kpatticha
Copy link
Contributor

kpatticha commented Nov 30, 2023

Checks is failing, but I get a "not found" when trying to view the details...

image

it's due to translations, you can fix it by running
node scripts/i18n_check.js --fix

Copy link
Contributor

@kpatticha kpatticha left a comment

Choose a reason for hiding this comment

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

I've identified two potential blockers that need to be addressed before merging, as well as a few UI enhancements that can be tackled separately.

  1. Crash details page is not accessible due to the params, it's an easy fix
  2. The filters don't persist between the overview and the details page.
Screen.Recording.2023-11-30.at.11.36.49.mov

I strongly recommend addressing the UI enhancements separately to avoid expanding the scope of the pull request.

  1. Changing the filters some of the charts are not displaying any loaders
  2. Most affected chart has no empty state so it looks like broken where there are no data. That was even before not related to this PR :D
  3. Layout of the crash/error details page is not consistent with the overview in terms of spaces and size of the panels

@kpatticha kpatticha changed the title Mobile errors [APM] Mobile crashes & errors Dec 1, 2023
Copy link
Contributor

@kpatticha kpatticha left a comment

Choose a reason for hiding this comment

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

The "Crash type" link is broken when clicked; it needs to align with the errors table. (I think clicking on the type is updates the kuery)

Screen.Recording.2023-12-01.at.15.44.53.mov

Note

It seems inconsistent that we show the error rate for the error tab while displaying crash occurrences (in line graph vs bars) for the crashes tab, even though both tables present the occurrences in the last column. We could discuss and improve in a follow up

@bryce-b bryce-b enabled auto-merge (squash) December 1, 2023 17:14
@bryce-b bryce-b merged commit 33c74ae into elastic:main Dec 1, 2023
33 checks passed
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1530 1546 +16

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 184 188 +4

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.7MB 3.7MB +26.0KB

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 120 127 +7

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
apm 36.7KB 36.7KB +49.0B
Unknown metric groups

API count

id before after diff
@kbn/apm-synthtrace-client 184 188 +4

ESLint disabled line counts

id before after diff
apm 75 77 +2

Total ESLint disabled count

id before after diff
apm 89 91 +2

History

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

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:cypress-record cypress test runs will be recorded to the cypress dashboard apm:review 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.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Enable the Errors & Crashes tab