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(integrations): Add HTTPClient integration #6500

Merged
merged 9 commits into from Jan 5, 2023

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Dec 12, 2022

Resolves: #6282

Implemented HTTPClient integration for @sentry/browser SDK.

Implementation Guide
Response Context Spec

This integration intercepts Fetch API and XHR requests to get the response (and request) information, and creates / captures events if there is a failed (according to the user's configuration) request.

Summary:

  • This integration wraps XMLHttpRequest and fetch utilities, which can also be wrapped by other Sentry - non-Sentry utilities before or after it.
  • Due to the limitations of both Fetch API and XHR, the cookie and header collection for both requests and responses is the best effort. Certain headers may be missing here in the integration-created event, even if they exist in the original request / response.
  • The integration is opt-in and should be initialised as below to work:
Sentry.init({
  dsn: 'https://public@dsn.ingest.sentry.io/1337',
  integrations: [
    new Sentry.Integrations.HttpClient({
      // This array can contain tuples of `[begin, end]` (both inclusive), 
      // Single status codes, or a combinations of both.
      // default: [[500, 599]]
      failedRequestStatusCodes: [[500, 505], 507],

      // This array can contain Regexes or strings, or combinations of both.
      // default: [/.*/]
      failedRequestTargets: [/ab+c/, 'http://example.com/api/test'],
    }),
  ],
  // This is required for capturing headers and cookies. 
  // Without it, the `event.request` and `event.contexts.response` objects
  // will not include `headers` and `cookies`.  
  sendDefaultPii: true,
});
  • The created events do not contain a stacktrace.
  • body_size is calculated only if the Content-Length header is available in response headers.

fetch-specific Notes:

  • Including body inside event.request object is not implemented here. It's not available at all for XHR requests, but here it's possible. Size limits and when to leave it out should be discussed, if we are including it.
  • This implementation recreates a Request object from arguments of the fetch function. Not sure if there are any concerns about performance, but this is the most straightforward way I could find to replicate the original request object.
  • Synthetically created request and native response objects are added to the event as hints, as described in the spec.

XHR-specific Notes:

  • Request cookies are not accessible from XHR requests. We're intercepting setRequestHeader method, which is only used for user or library-defined headers. And setting Cookie with setRequestHeader is not allowed in browsers. due to security reasons.
  • We also don't have a native request and response objects for XHR requests, so events created from failed XHR requests, do not contain hints.

@onurtemizkan onurtemizkan marked this pull request as draft December 12, 2022 15:28
@github-actions
Copy link
Contributor

github-actions bot commented Dec 12, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.84 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 61.45 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.62 KB (+0.04% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 54.99 KB (+0.04% 🔺)
@sentry/browser - Webpack (gzipped + minified) 20.39 KB (+0.02% 🔺)
@sentry/browser - Webpack (minified) 66.63 KB (+0.03% 🔺)
@sentry/react - Webpack (gzipped + minified) 20.41 KB (+0.02% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.66 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.82 KB (+0.04% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.25 KB (+0.01% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.99 KB (-0.01% 🔽)
@sentry/replay - Webpack (gzipped + minified) 38.24 KB (-0.02% 🔽)

@onurtemizkan onurtemizkan marked this pull request as ready for review December 13, 2022 15:37
@lforst lforst self-requested a review December 19, 2022 13:42
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

First pass. Already looks very good to me!

packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/types/src/context.ts Outdated Show resolved Hide resolved
packages/browser/src/sdk.ts Outdated Show resolved Hide resolved
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

One more pass, after this I think we're good :)

packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
packages/browser/src/integrations/httpclient.ts Outdated Show resolved Hide resolved
@smeubank
Copy link
Member

smeubank commented Jan 4, 2023

we need to discuss this bundle size bump and why it is so much on the CDN, if we can get a strategy to reduce it

@lforst
Copy link
Member

lforst commented Jan 4, 2023

@onurtemizkan I completely missed the bundle size impact. Can we move this integration into the @sentry/integrations package?

@lforst lforst changed the title feat(browser): HTTPClient Integration feat(integrations): HTTPClient Integration Jan 5, 2023
@lforst lforst changed the title feat(integrations): HTTPClient Integration feat(integrations): Add HTTPClient integration Jan 5, 2023
@lforst lforst merged commit afb3700 into master Jan 5, 2023
@lforst lforst deleted the onur/http-client-errors branch January 5, 2023 12:38
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.

HTTP Client Errors
4 participants