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

ref(browser): Use Proxy for XHR instrumentation #12212

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

lforst
Copy link
Member

@lforst lforst commented May 24, 2024

Fixes #12208

The implementation with proxy should be "more" transparent with regards to things like XMLHttpRequest.prototype.open.toString() for example.

Note: Proxies have less support than the previous implementation but it is still within our policy

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Let's please add a test!

Copy link
Contributor

github-actions bot commented May 24, 2024

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 22.52 KB +0.08% +17 B 🔺
@sentry/browser (incl. Tracing) 34.85 KB +0.02% +7 B 🔺
@sentry/browser (incl. Tracing, Replay) 71.21 KB +0.02% +11 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 64.48 KB +0.02% +12 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 75.57 KB +0.03% +18 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 88.29 KB +0.01% +8 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.13 KB +0.01% +5 B 🔺
@sentry/browser (incl. metrics) 26.83 KB +0.06% +16 B 🔺
@sentry/browser (incl. Feedback) 39.59 KB +0.06% +23 B 🔺
@sentry/browser (incl. sendFeedback) 27.18 KB +0.07% +17 B 🔺
@sentry/browser (incl. FeedbackAsync) 31.9 KB +0.04% +13 B 🔺
@sentry/react 25.28 KB +0.07% +16 B 🔺
@sentry/react (incl. Tracing) 37.83 KB -0.01% -2 B 🔽
@sentry/vue 26.66 KB +0.02% +5 B 🔺
@sentry/vue (incl. Tracing) 36.68 KB +0.03% +8 B 🔺
@sentry/svelte 22.65 KB +0.06% +12 B 🔺
CDN Bundle 23.76 KB +0.04% +8 B 🔺
CDN Bundle (incl. Tracing) 36.53 KB +0.06% +19 B 🔺
CDN Bundle (incl. Tracing, Replay) 70.86 KB +0.02% +12 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 76.16 KB +0.01% +7 B 🔺
CDN Bundle - uncompressed 69.61 KB -0.11% -74 B 🔽
CDN Bundle (incl. Tracing) - uncompressed 108.28 KB -0.07% -74 B 🔽
CDN Bundle (incl. Tracing, Replay) - uncompressed 219.66 KB -0.04% -74 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 232.85 KB -0.04% -74 B 🔽
@sentry/nextjs (client) 37.6 KB +0.04% +12 B 🔺
@sentry/sveltekit (client) 35.44 KB -0.02% -5 B 🔽
@sentry/node 115.71 KB - -
@sentry/node - without tracing 90.1 KB - -
@sentry/aws-serverless 99.51 KB - -

View base workflow run

@DerGernTod
Copy link

DerGernTod commented Aug 26, 2024

this fixes half of the issue in that it provides proper names for the xhr prototype functions. however, this is still a culprit:

Function.prototype.toString = function() {
    if (this.__sentry_original__) {
        return this.__sentry_original__.toString();
    }
    return origFunctionToString.apply(this);
}

this implementation per se is fine, unfortunately it conflicts with Function.prototype.name - which is not wrapped like this. so i guess this would also need something like

const origFunctionName = Function.prototype.name;
Object.defineProperty(Function.prototype, "name", {
  get() {
    if (this.__sentry_original__) {
      return this.__sentry_original__.name;
    }
    return origFunctionName.apply(this);
  }
});

this is just a poc, not sure if this code works, but that would bring consistency between Function.prototype.toString and Function.prototype.name i guess
edit: just noticed this doesn't work because the name property is not defined on the prototype, but only on the instance. this makes it a bit more difficult to make the behavior consistent...

@lforst lforst changed the title fix(browser): Don't stomp xhr function names ref(browser): Use Proxy for XHR instrumentation Aug 26, 2024
@lforst
Copy link
Member Author

lforst commented Aug 26, 2024

@DerGernTod makes sense. In your original issue, you're describing a symptom rather than the thing you would like to achieve.

I changed the implementation to be based on proxies. I think that should resolve most if not all of your problems. Can you confirm that?

@DerGernTod
Copy link

i tried your changes with our tests and looks like it works fine. nice, thanks!

@lforst lforst merged commit 2cbf64e into develop Aug 28, 2024
123 checks passed
@lforst lforst deleted the lforst-xhr-function-names branch August 28, 2024 08:59
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.

Wrapping of Function.prototype.toString causes inconsistent behavior of wrapped functions
4 participants