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

feat(rum-core): capture XHR/Fetch spans using resource timing #825

Merged
merged 10 commits into from
Jul 1, 2020

Conversation

vigneshshanmugam
Copy link
Member

@vigneshshanmugam vigneshshanmugam commented Jun 24, 2020

  • fix capture Fetch/XHR calls using ResourceTiming API #740
  • This PR addresses the problem when the agent is downloaded asynchronously and we miss the XHR and fetch spans that happened before the patch agent script kicks in. However, we wont be able to propagate the trace header and distributed tracing for these cases wont work.
  • Span names & types are not in sync with the patched ones as we want to distinguish them and would make it clear for the users that distributed tracing for the spans captured via resource.* wont work.
// resource
name - 'http://example.com/fetch'
type - 'resource.fetch'

// patched
name - 'GET http://example.com/fetch'
type - 'external.http'
  • Ignore the Intake API endpoint using resource name, as we could end up creating lots of spans for every events beacon and we cannot rely on bootstrap time and start time (browser bugs) for this endpoint alone.

  • Comparison logic is modified to take two things in to account

    1. Full URL's including query params as the web page may call the same endpoint with different queries and we have to capture them as separate spans
    2. Span start time as same request with same query params could happen before our patch kicked in and we have to capture them as different spans.

TODO

  • handle when /example request happened before agent script kicks in and another /example request caught via patching. These are two separate requests and we must create two spans instead of one.
  • Add more tests for the filtering logic with relative and absolute URL's.
  • Ignore intake API from spans

@apmmachine
Copy link
Contributor

apmmachine commented Jun 24, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Branch indexing]

  • Start Time: 2020-07-01T01:46:06.344+0000

  • Duration: 102 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 950
Skipped 10
Total 960

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 34 sec

    • Start Time: 2020-07-01T01:51:54.744+0000

    • log

  • Name: Start Elastic Stack 8.0.0-SNAPSHOT - @elastic/apm-rum-core - saucelabs

    • Description:

    • Duration: 8 min 0 sec

    • Start Time: 2020-07-01T02:19:04.506+0000

    • log

  • Name: Error signal

    • Description:

    • Duration: 0 min 0 sec

    • Start Time: 2020-07-01T02:27:04.926+0000

    • log

  • Name: Error signal

    • Description: The test failed

    • Duration: 0 min 0 sec

    • Start Time: 2020-07-01T03:12:26.171+0000

    • log

Copy link
Contributor

@hmdhk hmdhk left a comment

Choose a reason for hiding this comment

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

Thanks @vigneshshanmugam

A simpler solution for this issue might be to store the time we patch XHR (execution time) and use that time to capture spans from the resource timing, i.e. all the resources that have their start time before our execution time should be captured from the resource timing and all the rest are captured by the xhr patch. this would avoid having to loop over the spans to find the unique ones. Have you considered this solution?

side note: the execution time can be stored in the state object that will be introduced with #813

@vigneshshanmugam
Copy link
Member Author

vigneshshanmugam commented Jun 29, 2020

@jahtalab Nice suggestion 👍, I had the similar thought initially(not now) when i started working on RUM on why we were using filterUrls instead of patch time. I thought the reason we didn't go for that solution was due to two reasons.

  1. We cannot rely on Resource timing startTime since it can be buggy in browsers.
  2. Users can disable instrumentation of XHR and Fetch.

But as you said, It's simpler solution without doing the URL and for loop dance. ? I am happy with modifying the PR. WDYT?

@vigneshshanmugam vigneshshanmugam force-pushed the support-api-calls branch 2 times, most recently from 483619b to 44f42e1 Compare June 29, 2020 13:15
@vigneshshanmugam vigneshshanmugam requested a review from hmdhk June 29, 2020 13:17
@apmmachine
Copy link
Contributor

apmmachine commented Jun 29, 2020

📦 Bundlesize report

Filename Size(bundled) Size(gzip) Diff(gzip)
elastic-apm-opentracing.umd.min.js 59.5 KiB 19.1 KiB
elastic-apm-rum.umd.min.js 53.6 KiB 17.6 KiB

@vigneshshanmugam vigneshshanmugam requested a review from hmdhk June 30, 2020 15:12
Copy link
Contributor

@hmdhk hmdhk left a comment

Choose a reason for hiding this comment

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

@vigneshshanmugam , Have you looked at the tests? is there an issue?

@vigneshshanmugam
Copy link
Member Author

@jahtalab Its just travis. The tests passed though- https://travis-ci.org/github/elastic/apm-agent-rum-js/builds/703572041. I am going to ignore it.

@hmdhk hmdhk merged commit 5983e71 into elastic:master Jul 1, 2020
@vigneshshanmugam vigneshshanmugam deleted the support-api-calls branch July 1, 2020 12:31
v1v added a commit to v1v/apm-agent-rum-js that referenced this pull request Jul 3, 2020
* 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
…c#825)

* feat(rum-core): capture XHR/Fetch using resource timing

* chore: add tests and update fixtures

* chore: filter intake api from spans

* chore: handle duplicate url fetch before patch

* chore: add more tests for fitering logic

* chore: modify the logic as per review

* chore: fix  test

* chore: move to state object and fix test

* chore: add guard for patch time

* chore: address review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

capture Fetch/XHR calls using ResourceTiming API
3 participants