feat: Send transactions in envelopes #2553

rhcarvalho · 2020-04-22T15:41:07Z

The new Envelope endpoint and format is what we want to use for transactions going forward.

Apart from transactions as Envelopes, we unify how @sentry/browser and @sentry/node authenticate requests for ease of maintenance. Now all requests include the public key as a query string.

rhcarvalho · 2020-04-22T15:42:11Z

This is crude, but wanted to get the ball rolling early.

@getsentry/team-webplatform

packages/browser/src/transports/fetch.ts

packages/browser/src/transports/xhr.ts

packages/node/src/transports/base.ts

rhcarvalho · 2020-04-28T22:23:33Z

packages/node/src/transports/base.ts

    const headers = {
-      ...this._api.getRequestHeaders(SDK_NAME, SDK_VERSION),
+      // The auth headers are not included because auth is done via query string to match @sentry/browser.
+      //...this._api.getRequestHeaders(SDK_NAME, SDK_VERSION)


This is a proposition. Without it, this._api.getRequestHeaders needs to know that the Content-Type header is not always application/json anymore, but depends on the event type (error != transaction).

rhcarvalho · 2020-04-28T22:24:49Z

packages/node/src/transports/base.ts

-    const { hostname, pathname: path, port, protocol } = addr;
+    const { hostname, port, protocol } = url;
+    // See https://github.com/nodejs/node/blob/38146e717fed2fabe3aacb6540d839475e0ce1c6/lib/internal/url.js#L1268-L1290
+    const path = `${url.pathname || ''}${url.search || ''}`;


The helper in node includes the query string in the path, and we should too -- otherwise we'd miss the auth info.

rhcarvalho · 2020-04-28T22:25:50Z

packages/core/src/api.ts

@@ -17,29 +17,54 @@ export class API {
    return this._dsnObject;
  }

-  /** Returns a string with auth headers in the url to the store endpoint. */


This was the documentation of getStoreEndpoint! Fixed, it does not include auth.

packages/browser/src/integrations/breadcrumbs.ts

rhcarvalho · 2020-04-28T22:34:17Z

packages/browser/src/transports/base.ts

@@ -5,15 +5,19 @@ import { PromiseBuffer, SentryError } from '@sentry/utils';
 /** Base Transport class implementation */
 export abstract class BaseTransport implements Transport {
  /**
-   * @inheritDoc
+   * @deprecated


This depends on whether a single transport will eventually have to deal with multiple endpoints like now, or whether we'll have independent transports, each having a fixed target endpoint URL.

I think we should stick with one transport with multiple methods for getting url. Although I'm not sure if we should deprecate this attribute just yet, as we are not using envelopes for errors.

I'm not sure if we should deprecate this attribute just yet, as we are not using envelopes for errors.

The transport now always use the _api attribute to fetch the appropriate endpoint. url is only accessed in tests and possibly user code (e.g. custom transport).

packages/core/src/api.ts

rhcarvalho · 2020-04-28T22:37:36Z

packages/core/src/api.ts

-  /** Returns an object that can be used in request headers. */
+  /** Returns an object that can be used in request headers.
+   *
+   * @deprecated in favor of `getStoreEndpointWithUrlEncodedAuth` and `getEnvelopeEndpointWithUrlEncodedAuth`.


getRequestHeaders as-is is problematic now because the headers depend on the event type.

rhcarvalho · 2020-04-28T23:01:50Z

@sentry/browser tests need some love, "AssertionError: expected false to equal true" is not helpful.

packages/core/src/api.ts

rhcarvalho · 2020-04-30T09:47:33Z

packages/node/test/transports/http.test.ts

-  expect(options.headers['X-Sentry-Auth']).toContain('sentry_key');
+  // expect(options.headers['X-Sentry-Auth']).toContain('sentry_version');
+  // expect(options.headers['X-Sentry-Auth']).toContain('sentry_client');
+  // expect(options.headers['X-Sentry-Auth']).toContain('sentry_key');


If we're okay with sending auth via query string in node, then we can get rid of these lines.

The downside of auth in the query string is increased likelihood of having the "public key" part of the DSN exposed in logs, etc. Since this is more like "identification" then real "authentication", I believe it is not a huge concern, while allowing us to have simpler code.

packages/core/src/api.ts

packages/node/src/transports/base.ts

rhcarvalho · 2020-05-07T11:52:37Z

Linter error refers to a file that was not changed in this PR https://travis-ci.com/github/getsentry/sentry-javascript/jobs/329385512#L375,
packages/browser/src/integrations/trycatch.ts changed in #2570 where the linter run was green 😕

update: fixing that file in #2574

rhcarvalho · 2020-05-07T11:55:50Z

packages/node/test/transports/http.test.ts

-  expect(options.headers['X-Sentry-Auth']).toContain('sentry_version');
-  expect(options.headers['X-Sentry-Auth']).toContain('sentry_client');
-  expect(options.headers['X-Sentry-Auth']).toContain('sentry_key');
+  expect(options.headers).not.toContain('X-Sentry-Auth'); // auth is part of the query string


Please note this change. In order to reuse code between browser and node, we always send auth as part of the query string (and never send sentry_client anymore -- full SDK info is in the event payload).

The new Envelope endpoint and format is what we want to use for transactions going forward.

… request feat: Set outgoing request Content-Type to '' Sentry's Relay server expects envelopes to have Content-Type: application/x-sentry-envelope however, that header would cause CORS preflight requests that we want to avoid. If we don't set the Content-Type header, browsers fill it in with text/plain. We prefer sending an empty string than setting an incorrect Content-Type. Relay accepts the empty string and treats the body as an envelope.

…reflight request"

Docs moved, here's the new location.

rhcarvalho · 2020-05-08T19:40:01Z

One down side of this is the reduced debuggability and redundant information in the payload.

Transaction in Envelope 📨

Browsers don’t know how to prettify the long line of the event JSON;
"event_id":"thirty-two-bytes" appears twice in the request body;
"type":"transaction" appears twice too; and
"sent_at" is the same as the sentry_timestamp part of the authentication header (which we don't send at the moment).

Error Event sent to `/store/`

HazAT

👍

Next item on the menu, category-based rate limiting.

rhcarvalho force-pushed the transactions-in-envelopes branch 2 times, most recently from eb8eebf to c8cb2c3 Compare April 23, 2020 12:07

HazAT reviewed Apr 24, 2020

View reviewed changes

packages/browser/src/transports/fetch.ts Outdated Show resolved Hide resolved

packages/browser/src/transports/xhr.ts Outdated Show resolved Hide resolved

rhcarvalho force-pushed the transactions-in-envelopes branch 2 times, most recently from 6400b6e to cd3045f Compare April 28, 2020 22:27

rhcarvalho commented Apr 28, 2020

View reviewed changes

rhcarvalho marked this pull request as ready for review April 28, 2020 22:40

rhcarvalho requested a review from kamilogorek as a code owner April 28, 2020 22:40

rhcarvalho force-pushed the transactions-in-envelopes branch 2 times, most recently from e1bbb70 to 76c8de0 Compare April 28, 2020 22:59

rhcarvalho force-pushed the transactions-in-envelopes branch 2 times, most recently from 193f0ed to 15c0c69 Compare April 29, 2020 12:46

rhcarvalho commented Apr 30, 2020

View reviewed changes

packages/core/src/api.ts Outdated Show resolved Hide resolved

rhcarvalho commented Apr 30, 2020

View reviewed changes

rhcarvalho changed the title ~~[WIP] feat: Send transactions in envelopes~~ feat: Send transactions in envelopes Apr 30, 2020

kamilogorek suggested changes Apr 30, 2020

View reviewed changes

packages/core/src/api.ts Outdated Show resolved Hide resolved

packages/core/src/api.ts Outdated Show resolved Hide resolved

packages/core/src/api.ts Show resolved Hide resolved

packages/node/src/transports/base.ts Outdated Show resolved Hide resolved

rhcarvalho mentioned this pull request May 6, 2020

Consistent sending of auth keys #2572

Closed

rhcarvalho force-pushed the transactions-in-envelopes branch from 5a8a92b to 3886e5f Compare May 7, 2020 10:45

rhcarvalho requested a review from kamilogorek May 7, 2020 11:52

rhcarvalho commented May 7, 2020

View reviewed changes

rhcarvalho requested a review from HazAT May 7, 2020 13:40

rhcarvalho force-pushed the transactions-in-envelopes branch from ab264dc to c40c043 Compare May 7, 2020 13:41

kamilogorek and others added 5 commits May 8, 2020 18:27

Allow transport to accept url for the request directly

c5fbedf

feat: Send transactions in envelopes

8afcb73

The new Envelope endpoint and format is what we want to use for transactions going forward.

Revert "DO NOT MERGE: setting the Content-Type to '' does trigger a p…

181781d

…reflight request"

misc: Add CHANGELOG entry

b0339da

rhcarvalho force-pushed the transactions-in-envelopes branch from c40c043 to b0339da Compare May 8, 2020 16:27

docs: Update the already broken link

d8d7569

Docs moved, here's the new location.

HazAT approved these changes May 11, 2020

View reviewed changes

rhcarvalho merged commit 8958f61 into getsentry:master May 11, 2020

rhcarvalho deleted the transactions-in-envelopes branch May 11, 2020 11:35

rhcarvalho mentioned this pull request Jun 22, 2020

feat: Send transactions in envelopes getsentry/sentry-python#729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Send transactions in envelopes #2553

feat: Send transactions in envelopes #2553

rhcarvalho commented Apr 22, 2020 •

edited

Loading

rhcarvalho commented Apr 22, 2020

rhcarvalho Apr 28, 2020

rhcarvalho Apr 28, 2020

rhcarvalho Apr 28, 2020

rhcarvalho Apr 28, 2020

kamilogorek May 8, 2020

rhcarvalho May 8, 2020

rhcarvalho Apr 28, 2020

rhcarvalho commented Apr 28, 2020

rhcarvalho Apr 30, 2020

rhcarvalho commented May 7, 2020 •

edited

Loading

rhcarvalho May 7, 2020

rhcarvalho commented May 8, 2020

HazAT left a comment

feat: Send transactions in envelopes #2553

feat: Send transactions in envelopes #2553

Conversation

rhcarvalho commented Apr 22, 2020 • edited Loading

rhcarvalho commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhcarvalho commented Apr 28, 2020

Choose a reason for hiding this comment

rhcarvalho commented May 7, 2020 • edited Loading

Choose a reason for hiding this comment

rhcarvalho commented May 8, 2020

Transaction in Envelope 📨

Error Event sent to /store/

HazAT left a comment

Choose a reason for hiding this comment

rhcarvalho commented Apr 22, 2020 •

edited

Loading

rhcarvalho commented May 7, 2020 •

edited

Loading

Error Event sent to `/store/`