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

🐛 [RUMF-876] Improve proxy behavior for xhr reuse #865

Merged
merged 22 commits into from
Jun 10, 2021

Conversation

amortemousque
Copy link
Contributor

@amortemousque amortemousque commented May 26, 2021

Detected issues

  • We can have a max stack size error when using a single instance of XMLHTTPRequest.

Changes

  • Ensure to attach our listeners only once per request.

Testing

Unit, Locally


I have gone over the contributing documentation.

@amortemousque amortemousque requested a review from a team as a code owner May 26, 2021 14:10
@codecov-commenter
Copy link

codecov-commenter commented May 26, 2021

Codecov Report

Attention: Patch coverage is 97.82609% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.84%. Comparing base (8f24278) to head (185cb70).
Report is 1696 commits behind head on main.

Current head 185cb70 differs from pull request most recent head 00a48fe

Please upload reports for the commit 00a48fe to get more accurate results.

Files with missing lines Patch % Lines
packages/core/src/browser/xhrProxy.ts 97.82% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #865      +/-   ##
==========================================
- Coverage   89.13%   88.84%   -0.30%     
==========================================
  Files          81       81              
  Lines        3791     3764      -27     
  Branches      848      834      -14     
==========================================
- Hits         3379     3344      -35     
- Misses        412      420       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/core/src/browser/xhrProxy.ts Outdated Show resolved Hide resolved
packages/core/src/browser/xhrProxy.ts Outdated Show resolved Hide resolved
packages/core/src/browser/xhrProxy.spec.ts Outdated Show resolved Hide resolved
@amortemousque amortemousque changed the title ⚡️ Xhr reuse possible [RUMF-876]⚡️ Xhr reuse possible May 27, 2021
@amortemousque amortemousque changed the title [RUMF-876]⚡️ Xhr reuse possible ⚡️ [RUMF-876] Xhr reuse possible May 27, 2021
@amortemousque amortemousque changed the title ⚡️ [RUMF-876] Xhr reuse possible 🐛 [RUMF-876] Xhr reuse possible May 31, 2021
@amortemousque amortemousque changed the title 🐛 [RUMF-876] Xhr reuse possible 🐛 [RUMF-876] Improve proxy behavior for xhr reuse Jun 2, 2021
packages/core/src/browser/xhrProxy.spec.ts Outdated Show resolved Hide resolved
packages/core/src/browser/xhrProxy.spec.ts Outdated Show resolved Hide resolved
packages/core/src/browser/xhrProxy.spec.ts Outdated Show resolved Hide resolved
packages/core/src/browser/xhrProxy.spec.ts Outdated Show resolved Hide resolved
packages/core/test/specHelper.ts Outdated Show resolved Hide resolved
packages/core/test/specHelper.ts Outdated Show resolved Hide resolved
packages/core/test/specHelper.ts Outdated Show resolved Hide resolved
test/e2e/scenario/rum/resources.scenario.ts Outdated Show resolved Hide resolved
packages/core/src/browser/xhrProxy.spec.ts Outdated Show resolved Hide resolved
packages/core/test/specHelper.ts Outdated Show resolved Hide resolved
packages/core/src/browser/xhrProxy.ts Show resolved Hide resolved
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer left a comment

Choose a reason for hiding this comment

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

LGTM

packages/core/src/browser/xhrProxy.ts Outdated Show resolved Hide resolved
packages/core/src/browser/xhrProxy.ts Outdated Show resolved Hide resolved
return originalXhrAbort.apply(this, arguments as any)
}

function reportXhr(xhr: BrowserXHR<XhrStartContext>, pendingContext: XhrStartContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reasoning for not retrieving the context from xhr 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, I can simplify

Comment on lines 152 to 158
function reportXhr(xhr: BrowserXHR<XhrStartContext>, pendingContext: XhrStartContext) {
const xhrCompleteContext: XhrCompleteContext = {
...pendingContext,
duration: elapsed(pendingContext.startClocks.timeStamp, timeStampNow()),
response: xhr.response as string | undefined,
status: xhr.status,
}
Copy link
Contributor

@bcaudan bcaudan Jun 9, 2021

Choose a reason for hiding this comment

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

what about reinforcing the design by having something like:

Suggested change
function reportXhr(xhr: BrowserXHR<XhrStartContext>, pendingContext: XhrStartContext) {
const xhrCompleteContext: XhrCompleteContext = {
...pendingContext,
duration: elapsed(pendingContext.startClocks.timeStamp, timeStampNow()),
response: xhr.response as string | undefined,
status: xhr.status,
}
function reportXhr(xhr: BrowserXHR<XhrCompleteContext>) {
xhr._datadog_xhr!.duration = elapsed(pendingContext.startClocks.timeStamp, timeStampNow())
...

Copy link
Contributor Author

@amortemousque amortemousque Jun 9, 2021

Choose a reason for hiding this comment

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

Good point.
On thing I see, by doing this, is that I have to cast like so reportXhr(this as BrowserXHR<XhrCompleteContext>) in sendXhr()
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems good enough to me

@amortemousque amortemousque merged commit 52c475d into main Jun 10, 2021
@bcaudan bcaudan deleted the XHR-reuse-possible branch September 13, 2021 08:06
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.

4 participants