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

feat: use page visibilityState for browser responsiveness check #813

Merged
merged 3 commits into from
Jun 29, 2020
Merged

feat: use page visibilityState for browser responsiveness check #813

merged 3 commits into from
Jun 29, 2020

Conversation

hmdhk
Copy link
Contributor

@hmdhk hmdhk commented Jun 11, 2020 •
edited
Loading

This PR adds document visibilityState to check for browser responsiveness. The bootstrap function is added to keep all side-effects in one place.

  • Remove visibility check in metrics.js

Closes #295

@hmdhk hmdhk requested a review from vigneshshanmugam June 11, 2020 12:31
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2020 •
edited
Loading

Codecov Report

Merging #813 into master will increase coverage by 0.20%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master     #813      +/-   ##
==========================================
+ Coverage   92.66%   92.87%   +0.20%     
==========================================
  Files          50       51       +1     
  Lines        2291     2511     +220     
  Branches      456      547      +91     
==========================================
+ Hits         2123     2332     +209     
- Misses        165      176      +11     
  Partials        3        3
Impacted Files Coverage Δ
...ges/rum-core/src/performance-monitoring/metrics.js 89.41% <93.33%> (-0.13%) ⬇️
.../src/performance-monitoring/transaction-service.js 91.82% <100.00%> (+1.13%) ⬆️
packages/rum-react/src/get-apm-route.js 95.83% <100.00%> (-4.17%) ⬇️
packages/rum-react/src/get-with-transaction.js 92.68% <100.00%> (+24.26%) ⬆️
...c/performance-monitoring/performance-monitoring.js 95.21% <0.00%> (-0.05%) ⬇️
packages/rum-core/src/common/constants.js 100.00% <0.00%> (ø)
...rum-core/src/performance-monitoring/transaction.js 100.00% <0.00%> (ø)
...s/rum-core/src/performance-monitoring/bootstrap.js 9.09% <0.00%> (ø)

@apmmachine
Copy link
Contributor

apmmachine commented Jun 11, 2020 •
edited
Loading

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #813 updated]

  • Start Time: 2020-06-29T15:41:22.041+0000

  • Duration: 76 min 19 sec

Test stats 🧪

Test Results
Failed 0
Passed 944
Skipped 10
Total 954

Steps errors

Expand to view the steps failures

  • Name: Bundlesize

    • Description: #!/bin/bash set -o pipefail npm run bundlesize|tee bundlesize.txt

    • Duration: 1 min 30 sec

    • Start Time: 2020-06-29T15:46:41.102+0000

    • log

  • Name: Start Elastic Stack 7.7.0 - @elastic/apm-rum - none

    • Description:

    • Duration: 5 min 36 sec

    • Start Time: 2020-06-29T15:55:27.763+0000

    • log

  • Name: Start Elastic Stack 8.0.0-SNAPSHOT - @elastic/apm-rum-angular - none

    • Description:

    • Duration: 4 min 9 sec

    • Start Time: 2020-06-29T15:55:54.283+0000

    • log

  • Name: Start Elastic Stack 7.0.0 - @elastic/apm-rum-angular - none

    • Description:

    • Duration: 5 min 11 sec

    • Start Time: 2020-06-29T15:55:24.918+0000

    • log

vigneshshanmugam
vigneshshanmugam reviewed Jun 15, 2020
View reviewed changes
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Added some comments @jahtalab, Lets merge the changes from metrics.js as well in the same PR.

packages/rum-core/src/performance-monitoring/bootstrap.js Outdated Show resolved Hide resolved
packages/rum-core/src/performance-monitoring/bootstrap.js Outdated Show resolved Hide resolved
packages/rum-core/src/performance-monitoring/bootstrap.js Outdated Show resolved Hide resolved
@hmdhk hmdhk force-pushed the refactor-responsiveness branch from 3f7928f to 3037628 Compare June 29, 2020 12:10
@hmdhk hmdhk requested a review from vigneshshanmugam June 29, 2020 12:12
vigneshshanmugam
vigneshshanmugam reviewed Jun 29, 2020
View reviewed changes
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Thanks @jahtalab Added some comments. Could you add some tests? You can modify the state like how its done for TBT and other metrics test.

packages/rum-core/src/performance-monitoring/metrics.js Show resolved Hide resolved
packages/rum-core/src/performance-monitoring/transaction-service.js Outdated Show resolved Hide resolved
packages/rum-core/src/performance-monitoring/transaction-service.js Outdated Show resolved Hide resolved
packages/rum-core/src/state.js Outdated Show resolved Hide resolved
packages/rum-core/src/bootstrap.js Outdated Show resolved Hide resolved
@hmdhk hmdhk requested a review from vigneshshanmugam June 29, 2020 15:41
@apmmachine
Copy link
Contributor

📦 Bundlesize report

Filename Size(bundled) Size(gzip) Diff(gzip)
elastic-apm-opentracing.umd.min.js 61059 B 19564 B ( -152 )
elastic-apm-rum.umd.min.js 55014 B 18075 B ( -148 )

@vigneshshanmugam vigneshshanmugam merged commit 9de8344 into elastic:master Jun 29, 2020
v1v added a commit to v1v/apm-agent-rum-js that referenced this pull request Jul 3, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/ci-cdn
6ed7471 
* upstream/master: (23 commits)
  feat(rum-core): capture XHR/Fetch spans using resource timing (elastic#825)
  docs: update set-up.asciidoc (elastic#814)
  chore: remove compressed size gh workflow (elastic#828)
  feat: use page visibilityState for browser responsiveness check (elastic#813)
  ci(jenkins): report bundlesize as a GitHub comment (elastic#826)
  docs: release notes for 5.2.1 (elastic#824)
  chore(release): publish
  fix(rum-core): protect aganist buggy navigation timing data (elastic#819)
  fix(rum-core): protect aganist buggy navigation timing data (elastic#819)
  chore(rum-core): use startTime for LCP marks (elastic#815)
  fix(rum-core): capture tbt after all task entries are observed (elastic#803)
  feat(rum-react): use correct path when route is path array (elastic#800)
  ci: enable benchmark on a PR basis (elastic#812)
  ci: use dockerLogs step (elastic#810)
  fix: env var invalid type (elastic#809)
  fix: workarount for elastic/beats#18858 (elastic#807)
  docs: add release notes for 5.2.0 (elastic#801)
  chore(release): publish
  fix(rum-core): consider user defined type of high precedence (elastic#798)
  fix(rum): use single instance of apm across all packages (elastic#796)
  ...
David-Development pushed a commit to David-Development/apm-agent-rum-js that referenced this pull request Oct 20, 2021
@jahtalab
feat: use page visibilityState for browser responsiveness check (elas…
69ac8b6 
…tic#813)

* feat: use page visibilityState for browser responsiveness check

* chore: merge state and env

merge bootstrap files

* chore: address review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vigneshshanmugam vigneshshanmugam vigneshshanmugam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

replace browserresponsive check in favor of page visibility checks
4 participants
@hmdhk @codecov-commenter @apmmachine @vigneshshanmugam