-
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
[User experience] Fix JS error rate #81512
Conversation
2de8bfb
to
4e9479d
Compare
Pinging @elastic/apm-ui (Team:apm) |
Pinging @elastic/uptime (Team:uptime) |
- Exclude fetchStart exists filter for page views chart query - Remove JS error rate from UI - Query all JS errors, not just page-load transactions
@shahzad31 This is ready for another look. Addressed your feedback and added the tweaks from #78926 (comment). |
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,
Waiting for e2e test !!
Hrm, functional test failure is unrelated 😞
|
@elasticmachine merge upstream |
@elasticmachine merge upstream Flaky maps tests have been skipped upstream, giving this another whirl 🤞 |
💛 Build succeeded, but was flaky
Test FailuresChrome UI Functional Tests.test/functional/apps/discover/_doc_table·ts.discover app discover doc table expand a document row "before each" hook for "should expand the detail row when the toggle arrow is clicked"Standard Out
Stack Trace
Metrics [docs]async chunks size
History
To update your PR or re-run it, just comment with: |
* Query adjustments for getClientMetrics * Remove error rate from JS errors section
* Query adjustments for getClientMetrics * Remove error rate from JS errors section
…kibana into alerts/convert-to-tm-intervals * 'alerts/convert-to-tm-intervals' of github.com:gmmorris/kibana: (88 commits) fixed jest APM Experiments settings (elastic#81554) [Resolver] Enable resolver test plugin tests (elastic#81339) Add TS project references for inspector (elastic#81792) Add uri decode to es_ui_shared and fix navigation issues with special characters (elastic#80835) [Fleet] Rename ingestManager translations fleet (elastic#81837) [Logs UI] Transmit and render array field values in log entries (elastic#81385) Audit Logging: use the original url (elastic#81282) [User experience] Fix JS error rate (elastic#81512) [UX] Add median/percentile info in titles (elastic#79824) Support export for SO with circular refs (elastic#81582) Get rid of global types (elastic#81739) [APM] Fix precommit script (elastic#81594) skips overview tests (elastic#81877) [Security Solution][Case] Fix connector's labeling (elastic#81824) Added simple test, which only covers successful case when edit happened right after task was complete previous execution [Maps] Fix EMS test (elastic#81856) [Security Solutions][Detections] - Fix bug, last response not showing for disabled rules (elastic#81783) skip flaky suite (elastic#81853) Fixed type checks and unit tests ...
Summary
This PR partially fixes #78926. It incorporates feedback from #78926 (comment) onwards.
Namely:
Make sure
exists: { field: 'transaction.marks.navigationTiming.fetchStart' }
doesn't exclude documents in thegetClientMetrics
function queries (so they can contribute towards the page views value).Derive
page views
from the hit count of the outer query, rather than using avalue_count
aggregation (orcardinality
ontransaction.id
).Note
The
getRumPageLoadTransactionsProjection
projection is used in a lot of places, so I've added acheckFetchStartFieldExists
param which is by default set totrue
. I didn't want to add a separate projection just for thegetClientMetrics
query. This can be changed, but was the easiest way forward. There's a comment alongside theexists
that statesAdding this filter to cater for some inconsistent rum data not available on aggregated transactions
but it's not clear what that RUM data is, and so this should be applied to the smallest piece possible.