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.

Sign up for GitHub

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

Jump to bottom

feat(core): Deprecate API class #4281

Merged
merged 8 commits into from
Dec 14, 2021
Merged

feat(core): Deprecate API class #4281

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
77a8a28
feat(core): Deprecate API class
AbhiPrasad Dec 14, 2021
0672c71
Convert transports to use functions instead of API class
AbhiPrasad Dec 14, 2021
3bd4301
fix tests
AbhiPrasad Dec 14, 2021
c1d6f8b
fix request tests
AbhiPrasad Dec 14, 2021
fed9ce6
pass down tunnel properly
AbhiPrasad Dec 14, 2021
f8754e1
eslint disable deprecation
AbhiPrasad Dec 14, 2021
37e83a4
only check for tunnel once
AbhiPrasad Dec 14, 2021
075075f
Merge branch 'master' into abhi-deprecate-api-class
AbhiPrasad Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions packages/browser/src/transports/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { API } from '@sentry/core';
import {
APIDetails,
getEnvelopeEndpointWithUrlEncodedAuth,
getStoreEndpointWithUrlEncodedAuth,
initAPIDetails,
} from '@sentry/core';
import {
Event,
Outcome,
Expand Down Expand Up @@ -38,7 +43,7 @@ export abstract class BaseTransport implements Transport {
public url: string;

/** Helper to get Sentry API endpoints. */
protected readonly _api: API;
protected readonly _api: APIDetails;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is okay but generally I think this design still makes it hard for us to make improvements later. Unfortunately the subclassing situation will always be bad for minification but quite user friendly for custom transports, so it might be acceptable to do it this way.

In Python as an example we push the original options in (there are no transport options) and the transports pull from there what they need. We should probably not change this right now, but longer term I think that might be preferable here as well.

Copy link
Member Author

@AbhiPrasad AbhiPrasad Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I completely agree - but we can revisit this when we examine transports more holistically later on. I kept this pattern primarily to minimize the diff and possibilities for breakage.

original options in (there are no transport options) and the transports pull from there what they need

This is the direction we should head in, actually we might be able to pull it off before the major


/** A simple buffer holding all requests. */
protected readonly _buffer: PromiseBuffer<SentryResponse> = new PromiseBuffer(30);
Expand All @@ -49,9 +54,9 @@ export abstract class BaseTransport implements Transport {
protected _outcomes: { [key: string]: number } = {};

public constructor(public options: TransportOptions) {
this._api = new API(options.dsn, options._metadata, options.tunnel);
this._api = initAPIDetails(options.dsn, options._metadata, options.tunnel);
// eslint-disable-next-line deprecation/deprecation
this.url = this._api.getStoreEndpointWithUrlEncodedAuth();
this.url = getStoreEndpointWithUrlEncodedAuth(this._api.dsn);

if (this.options.sendClientReports && global.document) {
global.document.addEventListener('visibilitychange', () => {
Expand Down Expand Up @@ -112,9 +117,9 @@ export abstract class BaseTransport implements Transport {

logger.log(`Flushing outcomes:\n${JSON.stringify(outcomes, null, 2)}`);

const url = this._api.getEnvelopeEndpointWithUrlEncodedAuth();
const url = getEnvelopeEndpointWithUrlEncodedAuth(this._api.dsn, this._api.tunnel);
// Envelope header is required to be at least an empty object
const envelopeHeader = JSON.stringify({ ...(this.options.tunnel && { dsn: this._api.getDsn().toString() }) });
const envelopeHeader = JSON.stringify({ ...(this._api.tunnel && { dsn: this._api.dsn.toString() }) });
const itemHeaders = JSON.stringify({
type: 'client_report',
});
Expand Down
150 changes: 90 additions & 60 deletions packages/core/src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,26 @@ import { Dsn, urlEncode } from '@sentry/utils';

const SENTRY_API_VERSION = '7';

/**
* Stores details about a Sentry SDK
*/
export interface APIDetails {
/** The DSN as passed to Sentry.init() */
initDsn: DsnLike;
/** Metadata about the SDK (name, version, etc) for inclusion in envelope headers */
metadata: SdkMetadata;
/** The internally used Dsn object. */
readonly dsn: Dsn;
/** The envelope tunnel to use. */
readonly tunnel?: string;
}

/**
* Helper class to provide urls, headers and metadata that can be used to form
* different types of requests to Sentry endpoints.
* Supports both envelopes and regular event requests.
*
* @deprecated Please use APIDetails
**/
export class API {
/** The DSN as passed to Sentry.init() */
Expand Down Expand Up @@ -41,13 +57,12 @@ export class API {

/** Returns the prefix to construct Sentry ingestion API endpoints. */
public getBaseApiEndpoint(): string {
const dsn = this.getDsn();
return getBaseApiEndpoint(dsn);
return getBaseApiEndpoint(this._dsnObject);
}

/** Returns the store endpoint URL. */
public getStoreEndpoint(): string {
return this._getIngestEndpoint('store');
return getStoreEndpoint(this._dsnObject);
}

/**
Expand All @@ -56,7 +71,7 @@ export class API {
* Sending auth as part of the query string and not as custom HTTP headers avoids CORS preflight requests.
*/
public getStoreEndpointWithUrlEncodedAuth(): string {
return `${this.getStoreEndpoint()}?${this._encodedAuth()}`;
return getStoreEndpointWithUrlEncodedAuth(this._dsnObject);
}

/**
Expand All @@ -65,64 +80,18 @@ export class API {
* Sending auth as part of the query string and not as custom HTTP headers avoids CORS preflight requests.
*/
public getEnvelopeEndpointWithUrlEncodedAuth(): string {
if (this.forceEnvelope()) {
return this._tunnel as string;
}

return `${this._getEnvelopeEndpoint()}?${this._encodedAuth()}`;
}

/** Returns only the path component for the store endpoint. */
public getStoreEndpointPath(): string {
const dsn = this.getDsn();
return `${dsn.path ? `/${dsn.path}` : ''}/api/${dsn.projectId}/store/`;
}

/**
* Returns an object that can be used in request headers.
* This is needed for node and the old /store endpoint in sentry
*/
public getRequestHeaders(clientName: string, clientVersion: string): { [key: string]: string } {
// CHANGE THIS to use metadata but keep clientName and clientVersion compatible
const dsn = this.getDsn();
const header = [`Sentry sentry_version=${SENTRY_API_VERSION}`];
header.push(`sentry_client=${clientName}/${clientVersion}`);
header.push(`sentry_key=${dsn.publicKey}`);
if (dsn.pass) {
header.push(`sentry_secret=${dsn.pass}`);
}
return {
'Content-Type': 'application/json',
'X-Sentry-Auth': header.join(', '),
};
}

/** Returns the envelope endpoint URL. */
private _getEnvelopeEndpoint(): string {
return this._getIngestEndpoint('envelope');
}

/** Returns the ingest API endpoint for target. */
private _getIngestEndpoint(target: 'store' | 'envelope'): string {
if (this._tunnel) {
return this._tunnel;
}
const base = this.getBaseApiEndpoint();
const dsn = this.getDsn();
return `${base}${dsn.projectId}/${target}/`;
return getEnvelopeEndpointWithUrlEncodedAuth(this._dsnObject, this._tunnel);
}
}

/** Returns a URL-encoded string with auth config suitable for a query string. */
private _encodedAuth(): string {
const dsn = this.getDsn();
const auth = {
// We send only the minimum set of required information. See
// https://github.com/getsentry/sentry-javascript/issues/2572.
sentry_key: dsn.publicKey,
sentry_version: SENTRY_API_VERSION,
};
return urlEncode(auth);
}
/** Initializes API Details */
export function initAPIDetails(dsn: DsnLike, metadata?: SdkMetadata, tunnel?: string): APIDetails {
return {
initDsn: dsn,
metadata: metadata || {},
dsn: new Dsn(dsn),
tunnel,
} as APIDetails;
}

/** Returns the prefix to construct Sentry ingestion API endpoints. */
Expand All @@ -132,6 +101,67 @@ function getBaseApiEndpoint(dsn: Dsn): string {
return `${protocol}//${dsn.host}${port}${dsn.path ? `/${dsn.path}` : ''}/api/`;
}

/** Returns the ingest API endpoint for target. */
function _getIngestEndpoint(dsn: Dsn, target: 'store' | 'envelope'): string {
return `${getBaseApiEndpoint(dsn)}${dsn.projectId}/${target}/`;
}

/** Returns a URL-encoded string with auth config suitable for a query string. */
function _encodedAuth(dsn: Dsn): string {
return urlEncode({
// We send only the minimum set of required information. See
// https://github.com/getsentry/sentry-javascript/issues/2572.
sentry_key: dsn.publicKey,
sentry_version: SENTRY_API_VERSION,
});
}

/** Returns the store endpoint URL. */
function getStoreEndpoint(dsn: Dsn): string {
return _getIngestEndpoint(dsn, 'store');
}

/**
* Returns the store endpoint URL with auth in the query string.
*
* Sending auth as part of the query string and not as custom HTTP headers avoids CORS preflight requests.
*/
export function getStoreEndpointWithUrlEncodedAuth(dsn: Dsn): string {
return `${getStoreEndpoint(dsn)}?${_encodedAuth(dsn)}`;
}

/** Returns the envelope endpoint URL. */
function _getEnvelopeEndpoint(dsn: Dsn): string {
return _getIngestEndpoint(dsn, 'envelope');
}

/**
* Returns the envelope endpoint URL with auth in the query string.
*
* Sending auth as part of the query string and not as custom HTTP headers avoids CORS preflight requests.
*/
export function getEnvelopeEndpointWithUrlEncodedAuth(dsn: Dsn, tunnel?: string): string {
return tunnel ? tunnel : `${_getEnvelopeEndpoint(dsn)}?${_encodedAuth(dsn)}`;
}

/**
* Returns an object that can be used in request headers.
* This is needed for node and the old /store endpoint in sentry
*/
export function getRequestHeaders(dsn: Dsn, clientName: string, clientVersion: string): { [key: string]: string } {
// CHANGE THIS to use metadata but keep clientName and clientVersion compatible
const header = [`Sentry sentry_version=${SENTRY_API_VERSION}`];
header.push(`sentry_client=${clientName}/${clientVersion}`);
header.push(`sentry_key=${dsn.publicKey}`);
if (dsn.pass) {
header.push(`sentry_secret=${dsn.pass}`);
}
return {
'Content-Type': 'application/json',
'X-Sentry-Auth': header.join(', '),
};
}

/** Returns the url to the report dialog endpoint. */
export function getReportDialogEndpoint(
dsnLike: DsnLike,
Expand Down
11 changes: 10 additions & 1 deletion packages/core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,16 @@ export {
withScope,
} from '@sentry/minimal';
export { addGlobalEventProcessor, getCurrentHub, getHubFromCarrier, Hub, makeMain, Scope } from '@sentry/hub';
export { API, getReportDialogEndpoint } from './api';
export {
// eslint-disable-next-line deprecation/deprecation
API,
APIDetails,
getEnvelopeEndpointWithUrlEncodedAuth,
getStoreEndpointWithUrlEncodedAuth,
getRequestHeaders,
initAPIDetails,
getReportDialogEndpoint,
} from './api';
export { BaseClient } from './baseclient';
export { BackendClass, BaseBackend } from './basebackend';
export { eventToSentryRequest, sessionToSentryRequest } from './request';
Expand Down
20 changes: 11 additions & 9 deletions packages/core/src/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { Event, SdkInfo, SentryRequest, SentryRequestType, Session, SessionAggregates } from '@sentry/types';

import { API } from './api';
import { APIDetails, getEnvelopeEndpointWithUrlEncodedAuth, getStoreEndpointWithUrlEncodedAuth } from './api';

/** Extract sdk info from from the API metadata */
function getSdkMetadataForEnvelopeHeader(api: API): SdkInfo | undefined {
function getSdkMetadataForEnvelopeHeader(api: APIDetails): SdkInfo | undefined {
if (!api.metadata || !api.metadata.sdk) {
return;
}
Expand All @@ -28,12 +28,12 @@ function enhanceEventWithSdkInfo(event: Event, sdkInfo?: SdkInfo): Event {
}

/** Creates a SentryRequest from a Session. */
export function sessionToSentryRequest(session: Session | SessionAggregates, api: API): SentryRequest {
export function sessionToSentryRequest(session: Session | SessionAggregates, api: APIDetails): SentryRequest {
const sdkInfo = getSdkMetadataForEnvelopeHeader(api);
const envelopeHeaders = JSON.stringify({
sent_at: new Date().toISOString(),
...(sdkInfo && { sdk: sdkInfo }),
...(api.forceEnvelope() && { dsn: api.getDsn().toString() }),
...(!!api.tunnel && { dsn: api.dsn.toString() }),
});
// I know this is hacky but we don't want to add `session` to request type since it's never rate limited
const type: SentryRequestType = 'aggregates' in session ? ('sessions' as SentryRequestType) : 'session';
Expand All @@ -44,15 +44,15 @@ export function sessionToSentryRequest(session: Session | SessionAggregates, api
return {
body: `${envelopeHeaders}\n${itemHeaders}\n${JSON.stringify(session)}`,
type,
url: api.getEnvelopeEndpointWithUrlEncodedAuth(),
url: getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel),
};
}

/** Creates a SentryRequest from an event. */
export function eventToSentryRequest(event: Event, api: API): SentryRequest {
export function eventToSentryRequest(event: Event, api: APIDetails): SentryRequest {
const sdkInfo = getSdkMetadataForEnvelopeHeader(api);
const eventType = event.type || 'event';
const useEnvelope = eventType === 'transaction' || api.forceEnvelope();
const useEnvelope = eventType === 'transaction' || !!api.tunnel;

const { transactionSampling, ...metadata } = event.debug_meta || {};
const { method: samplingMethod, rate: sampleRate } = transactionSampling || {};
Expand All @@ -65,7 +65,9 @@ export function eventToSentryRequest(event: Event, api: API): SentryRequest {
const req: SentryRequest = {
body: JSON.stringify(sdkInfo ? enhanceEventWithSdkInfo(event, api.metadata.sdk) : event),
type: eventType,
url: useEnvelope ? api.getEnvelopeEndpointWithUrlEncodedAuth() : api.getStoreEndpointWithUrlEncodedAuth(),
url: useEnvelope
? getEnvelopeEndpointWithUrlEncodedAuth(api.dsn, api.tunnel)
: getStoreEndpointWithUrlEncodedAuth(api.dsn),
};

// https://develop.sentry.dev/sdk/envelopes/
Expand All @@ -79,7 +81,7 @@ export function eventToSentryRequest(event: Event, api: API): SentryRequest {
event_id: event.event_id,
sent_at: new Date().toISOString(),
...(sdkInfo && { sdk: sdkInfo }),
...(api.forceEnvelope() && { dsn: api.getDsn().toString() }),
...(!!api.tunnel && { dsn: api.dsn.toString() }),
});
const itemHeaders = JSON.stringify({
type: eventType,
Expand Down
7 changes: 4 additions & 3 deletions packages/core/test/lib/api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable deprecation/deprecation */
import { Dsn } from '@sentry/utils';

import { API, getReportDialogEndpoint } from '../../src/api';
import { API, getReportDialogEndpoint, getRequestHeaders } from '../../src/api';

const ingestDsn = 'https://[email protected]:1234/subpath/123';
const dsnPublic = 'https://[email protected]:1234/subpath/123';
Expand All @@ -24,12 +25,12 @@ describe('API', () => {
});

test('getRequestHeaders', () => {
expect(new API(dsnPublic).getRequestHeaders('a', '1.0')).toMatchObject({
expect(getRequestHeaders(new Dsn(dsnPublic), 'a', '1.0')).toMatchObject({
'Content-Type': 'application/json',
'X-Sentry-Auth': expect.stringMatching(/^Sentry sentry_version=\d, sentry_client=a\/1\.0, sentry_key=abc$/),
});

expect(new API(legacyDsn).getRequestHeaders('a', '1.0')).toMatchObject({
expect(getRequestHeaders(new Dsn(legacyDsn), 'a', '1.0')).toMatchObject({
'Content-Type': 'application/json',
'X-Sentry-Auth': expect.stringMatching(
/^Sentry sentry_version=\d, sentry_client=a\/1\.0, sentry_key=abc, sentry_secret=123$/,
Expand Down
Loading