feat(core): Introduce New Transports API #4716

AbhiPrasad · 2022-03-14T14:43:58Z

Part 1 of #4660
Supercedes #4662

This PR introduces a functional transport based on what was discussed in #4660.

The transport is defined by an interface, which sets two basic functions, send and flush, which both interact directly with an internal PromiseBuffer data structure, a queue of requests that represents the data that needs to be sent to Sentry.

interface NewTransport {
  // If `$` is set, we know that this is a new transport.
  // TODO(v7): Remove this as we will no longer have split between
  // old and new transports.
  $: boolean;
  send(request: Envelope, category: TransportCategory): PromiseLike<TransportResponse>;
  flush(timeout: number): PromiseLike<boolean>;
}

send relies on an externally defined makeRequest function (that is passed into a makeTransport constructor) to make a request, and then return a status based on it. It also updates internal rate-limits according to the response from makeRequest. The status will be also used by the client in the future for client outcomes (we will extract client outcomes from the transport -> client because it living in the transport is not the best pattern).

send takes in an envelope, which means that for now, no errors will go through this transport when we update the client to use this new transport.

flush, flushes the promise buffer, pretty much how it worked before.

To make a custom transport (and how we'll be making fetch, xhr transports), you essentially define a makeRequest function.

function createFetchTransport(options: FetchTransportOptions): NewTransport {
  function makeRequest(request: TransportRequest): PromiseLike<TransportMakeRequestResponse> {
    return fetch(options.url, options.fetchOptions).then(response => {
      return response.text().then(body => ({
        body,
        headers: {
          'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'),
          'retry-after': response.headers.get('Retry-After'),
        },
        reason: response.statusText,
        statusCode: response.status,
      }));
    });
  }

  return createTransport({ bufferSize: options.bufferSize }, makeRequest);
}

^^ Look how clean that is!!

Resolves https://getsentry.atlassian.net/browse/WEB-662

github-actions · 2022-03-14T14:54:23Z

size-limit report

Path	Base Size (ebc9da5)	Current Size	Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.49 KB	19.49 KB	+0.01% 🔺
@sentry/browser - ES5 CDN Bundle (minified)	62.17 KB	62.17 KB	0%
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.11 KB	18.12 KB	+0.02% 🔺
@sentry/browser - ES6 CDN Bundle (minified)	55.5 KB	55.5 KB	0%
@sentry/browser - Webpack (gzipped + minified)	22.6 KB	22.6 KB	0%
@sentry/browser - Webpack (minified)	79.21 KB	79.21 KB	0%
@sentry/react - Webpack (gzipped + minified)	22.63 KB	22.63 KB	0%
@sentry/nextjs Client - Webpack (gzipped + minified)	47.6 KB	47.6 KB	0%
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.36 KB	25.36 KB	+0.01% 🔺
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	23.72 KB	23.72 KB	+0.01% 🔺

AbhiPrasad · 2022-03-18T13:31:19Z

packages/core/src/transports/base.ts

+
+export type TransportMakeRequestResponse = {
+  body?: string;
+  headers?: Record<string, string | null>;


debating if I need to make the rate limiting headers mandatory here.

When I read through the PR I instinctively asked myself the same thing. In my very uneducated opinion it should be non-optional.

Also: Is there a reason we allow null?

Yeah I will change this to be non-optional

We allow null because most getters for headers return null if not defined. For example, https://developer.mozilla.org/en-US/docs/Web/API/Headers/get

Lms24

This is looking good as far as I can tell. Checked out the branch and ran the tests. I just left a comment on the rate limit tests. Although, with our conversation from earlier in mind, feel free to ignore it if you want.

Lms24 · 2022-03-18T17:46:43Z

packages/core/test/lib/transports/base.test.ts

+        const retryAfterSeconds = 10;
+        const beforeLimit = Date.now();
+        const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000;
+        const afterLimit = beforeLimit + retryAfterSeconds * 1000;


Just a suggestion: Since these variables are identical in 5 tests, I think it would make sense to extract them into a helper function.

lforst

I really like the API - super clean! I left some comments on naming and a potential small bug (?). The tests also look great.

lforst · 2022-03-21T10:34:11Z

packages/core/src/transports/base.ts

+
+export type TransportMakeRequestResponse = {
+  body?: string;
+  headers?: Record<string, string | null>;


When I read through the PR I instinctively asked myself the same thing. In my very uneducated opinion it should be non-optional.

Also: Is there a reason we allow null?

packages/core/src/transports/base.ts

AbhiPrasad · 2022-03-21T16:02:02Z

In the spirit of moving fast, I'm going to merge this in and start work on the client side stuff. We can make improvements to this later on.

AbhiPrasad force-pushed the abhi-brand-new-transports branch from 58eec6a to a4fc0b3 Compare March 17, 2022 16:49

feat(core): Introduce New Transports API

f0b49b4

AbhiPrasad force-pushed the abhi-brand-new-transports branch from a4fc0b3 to f0b49b4 Compare March 17, 2022 16:57

AbhiPrasad marked this pull request as ready for review March 17, 2022 16:58

AbhiPrasad added 4 commits March 18, 2022 09:10

re-arrange

a8f56b8

remove required URL

cbde60d

fix lint

11a0a69

Merge branch 'master' into abhi-brand-new-transports

5690250

AbhiPrasad requested review from lforst, Lms24 and lobsterkatie March 18, 2022 08:44

kamilogorek self-requested a review March 18, 2022 12:13

AbhiPrasad commented Mar 18, 2022

View reviewed changes

make flush timeout optional

c842977

Lms24 approved these changes Mar 18, 2022

View reviewed changes

lforst reviewed Mar 21, 2022

View reviewed changes

AbhiPrasad added 3 commits March 21, 2022 10:18

Merge branch 'master' into abhi-brand-new-transports

058d00e

address PR feedback

606b76d

explicitly write out rate limit headers

b709e98

AbhiPrasad merged commit 76acf2f into master Mar 21, 2022

AbhiPrasad deleted the abhi-brand-new-transports branch March 21, 2022 16:02

AbhiPrasad added this to the Pre 7.0.0 Work milestone Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Introduce New Transports API #4716

feat(core): Introduce New Transports API #4716

AbhiPrasad commented Mar 14, 2022 •

edited

github-actions bot commented Mar 14, 2022 •

edited

AbhiPrasad Mar 18, 2022

lforst Mar 21, 2022

AbhiPrasad Mar 21, 2022

Lms24 left a comment

Lms24 Mar 18, 2022

AbhiPrasad Mar 21, 2022

lforst left a comment

lforst Mar 21, 2022

AbhiPrasad commented Mar 21, 2022

feat(core): Introduce New Transports API #4716

feat(core): Introduce New Transports API #4716

Conversation

AbhiPrasad commented Mar 14, 2022 • edited

github-actions bot commented Mar 14, 2022 • edited

size-limit report

AbhiPrasad Mar 18, 2022

Choose a reason for hiding this comment

lforst Mar 21, 2022

Choose a reason for hiding this comment

AbhiPrasad Mar 21, 2022

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

Lms24 Mar 18, 2022

Choose a reason for hiding this comment

AbhiPrasad Mar 21, 2022

Choose a reason for hiding this comment

lforst left a comment

Choose a reason for hiding this comment

lforst Mar 21, 2022

Choose a reason for hiding this comment

AbhiPrasad commented Mar 21, 2022

AbhiPrasad commented Mar 14, 2022 •

edited

github-actions bot commented Mar 14, 2022 •

edited