Skip to content

feat(integrations): Add HTTPClient integration - #6500

Merged
lforst merged 9 commits into
masterfrom
onur/http-client-errors
Jan 5, 2023
Merged

feat(integrations): Add HTTPClient integration#6500
lforst merged 9 commits into
masterfrom
onur/http-client-errors

Conversation

@onurtemizkan

@onurtemizkanonurtemizkan commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

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: [newSentry.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

github-actionsBot commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@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
onurtemizkanforce-pushed the onur/http-client-errors branch 2 times, most recently from 9f40a03 to 0eb8748CompareDecember 13, 2022 13:30
@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

@lforstlforst left a comment

Copy link
Copy Markdown
Contributor

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!

Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/types/src/context.ts Outdated
Comment threadpackages/browser/src/sdk.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated

@lforstlforst left a comment

Copy link
Copy Markdown
Contributor

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 :)

Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
Comment threadpackages/browser/src/integrations/httpclient.ts Outdated
@smeubank

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@onurtemizkan
onurtemizkanforce-pushed the onur/http-client-errors branch from 523ede7 to e8fb9cdCompareJanuary 4, 2023 17:22
@lforstlforst changed the title feat(browser): HTTPClient Integrationfeat(integrations): HTTPClient IntegrationJan 5, 2023
@lforstlforst changed the title feat(integrations): HTTPClient Integrationfeat(integrations): Add HTTPClient integrationJan 5, 2023
@lforst
lforst merged commit afb3700 into masterJan 5, 2023
@lforst
lforst deleted the onur/http-client-errors branch January 5, 2023 12:38
Sign up for freeto 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

@onurtemizkan@smeubank@lforst@Lms24