Skip to content

feat: Always assemble Envelopes #9101

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

Merged
merged 11 commits into from
Sep 27, 2023
Merged

feat: Always assemble Envelopes #9101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2b415ec
fix conflight
HazAT Sep 21, 2023
e49b078
feat: Add Spotlight Client
HazAT Sep 21, 2023
ab4fdd1
fix: Imports
HazAT Sep 21, 2023
c135853
ref: Move sidecar to Spotlight
HazAT Sep 21, 2023
278c6da
ref: Don't have a spotlight transport
HazAT Sep 23, 2023
1568e5b
Merge branch 'develop' into feat/spotlight
HazAT Sep 25, 2023
55d4895
ref: Make enabled consistent
HazAT Sep 26, 2023
c777b69
fix: Tests
HazAT Sep 26, 2023
3d61217
Merge branch 'develop' into feat/spotlight
HazAT Sep 26, 2023
7f83c35
ref: Remove try/catch
HazAT Sep 26, 2023
cb07217
Merge branch 'develop' into feat/spotlight
HazAT Sep 27, 2023
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
1 change: 1 addition & 0 deletions packages/browser/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
return;
}

// This is really the only place where we want to check for a DSN and only send outcomes then
if (!this._dsn) {
__DEBUG_BUILD__ && logger.log('No dsn provided, will not send outcomes');
return;
Expand Down
59 changes: 23 additions & 36 deletions packages/core/src/baseclient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
if (options.dsn) {
this._dsn = makeDsn(options.dsn);
} else {
__DEBUG_BUILD__ && logger.warn('No DSN provided, client will not do anything.');
__DEBUG_BUILD__ && logger.warn('No DSN provided, client will not send events.');
}

if (this._dsn) {
Expand Down Expand Up @@ -216,11 +216,6 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
* @inheritDoc
*/
public captureSession(session: Session): void {
if (!this._isEnabled()) {
__DEBUG_BUILD__ && logger.warn('SDK not enabled, will not capture session.');
return;
}

if (!(typeof session.release === 'string')) {
__DEBUG_BUILD__ && logger.warn('Discarded session because of missing or non-string release');
} else {
Expand Down Expand Up @@ -297,8 +292,8 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
/**
* Sets up the integrations
*/
public setupIntegrations(): void {
if (this._isEnabled() && !this._integrationsInitialized) {
public setupIntegrations(forceInitialize?: boolean): void {
Copy link
Member

Choose a reason for hiding this comment

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

l: Maybe it would be nicer to split this into to methods, than to have this argument:

public setupIntegrations(): void {
  if (this._isEnabled() && !this._integrationsInitialized) {
    this.initializeIntegrations(); // <-- naming TBD??
  }
}

public initializeIntegrations(): void {
  this._integrations = setupIntegrations(this, this._options.integrations);
  this._integrationsInitialized = true;
}

I guess the idea is that e.g. spotlight would initialize sentry integrations even if not enabled? So that could then just call client.initializeIntegration()?

Copy link
Member Author

@HazAT HazAT Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yep see https://github.com/getsentry/spotlight/pull/3/files#diff-0b5adbfe7b36e4ae2f479291e20152e33e940f7f265162d77f40f6bdb5da7405R78

will change it, like your proposal
actually, this might be worth doing when we rethink how integrations work
the problem is, I still want to make sure that calling this more than once doesn't setup integrations more than once

with this change, it's not guaranteed that an integration will only be called once

if ((forceInitialize && !this._integrationsInitialized) || (this._isEnabled() && !this._integrationsInitialized)) {
this._integrations = setupIntegrations(this, this._options.integrations);
this._integrationsInitialized = true;
}
Expand Down Expand Up @@ -338,34 +333,30 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
public sendEvent(event: Event, hint: EventHint = {}): void {
this.emit('beforeSendEvent', event, hint);

if (this._dsn) {
let env = createEventEnvelope(event, this._dsn, this._options._metadata, this._options.tunnel);

for (const attachment of hint.attachments || []) {
env = addItemToEnvelope(
env,
createAttachmentEnvelopeItem(
attachment,
this._options.transportOptions && this._options.transportOptions.textEncoder,
),
);
}
let env = createEventEnvelope(event, this._dsn, this._options._metadata, this._options.tunnel);

const promise = this._sendEnvelope(env);
if (promise) {
promise.then(sendResponse => this.emit('afterSendEvent', event, sendResponse), null);
}
for (const attachment of hint.attachments || []) {
env = addItemToEnvelope(
env,
createAttachmentEnvelopeItem(
attachment,
this._options.transportOptions && this._options.transportOptions.textEncoder,
),
);
}

const promise = this._sendEnvelope(env);
if (promise) {
promise.then(sendResponse => this.emit('afterSendEvent', event, sendResponse), null);
}
}

/**
* @inheritDoc
*/
public sendSession(session: Session | SessionAggregates): void {
if (this._dsn) {
const env = createSessionEnvelope(session, this._dsn, this._options._metadata, this._options.tunnel);
void this._sendEnvelope(env);
}
const env = createSessionEnvelope(session, this._dsn, this._options._metadata, this._options.tunnel);
void this._sendEnvelope(env);
}

/**
Expand Down Expand Up @@ -531,9 +522,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
});
}

/** Determines whether this SDK is enabled and a valid Dsn is present. */
/** Determines whether this SDK is enabled and a transport is present. */
protected _isEnabled(): boolean {
return this.getOptions().enabled !== false && this._dsn !== undefined;
return this.getOptions().enabled !== false && this._transport !== undefined;
}

/**
Expand Down Expand Up @@ -635,10 +626,6 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
const options = this.getOptions();
const { sampleRate } = options;

if (!this._isEnabled()) {
return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.', 'log'));
}

const isTransaction = isTransactionEvent(event);
const isError = isErrorEvent(event);
const eventType = event.type || 'error';
Expand Down Expand Up @@ -738,9 +725,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
* @inheritdoc
*/
protected _sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
if (this._transport && this._dsn) {
this.emit('beforeEnvelope', envelope);
this.emit('beforeEnvelope', envelope);

if (this._isEnabled() && this._transport) {
return this._transport.send(envelope).then(null, reason => {
__DEBUG_BUILD__ && logger.error('Error while sending event:', reason);
});
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/envelope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@ function enhanceEventWithSdkInfo(event: Event, sdkInfo?: SdkInfo): Event {
/** Creates an envelope from a Session */
export function createSessionEnvelope(
session: Session | SessionAggregates,
dsn: DsnComponents,
dsn?: DsnComponents,
metadata?: SdkMetadata,
tunnel?: string,
): SessionEnvelope {
const sdkInfo = getSdkMetadataForEnvelopeHeader(metadata);
const envelopeHeaders = {
sent_at: new Date().toISOString(),
...(sdkInfo && { sdk: sdkInfo }),
...(!!tunnel && { dsn: dsnToString(dsn) }),
...(!!tunnel && dsn && { dsn: dsnToString(dsn) }),
};

const envelopeItem: SessionItem =
Expand All @@ -58,7 +58,7 @@ export function createSessionEnvelope(
*/
export function createEventEnvelope(
event: Event,
dsn: DsnComponents,
dsn?: DsnComponents,
metadata?: SdkMetadata,
tunnel?: string,
): EventEnvelope {
Expand Down
45 changes: 6 additions & 39 deletions packages/core/test/lib/base.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,30 +398,6 @@ describe('BaseClient', () => {
});

describe('captureEvent() / prepareEvent()', () => {
test('skips when disabled', () => {
expect.assertions(1);

const options = getDefaultTestClientOptions({ enabled: false, dsn: PUBLIC_DSN });
const client = new TestClient(options);
const scope = new Scope();

client.captureEvent({}, undefined, scope);

expect(TestClient.instance!.event).toBeUndefined();
});

test('skips without a Dsn', () => {
expect.assertions(1);

const options = getDefaultTestClientOptions({});
const client = new TestClient(options);
const scope = new Scope();

client.captureEvent({}, undefined, scope);

expect(TestClient.instance!.event).toBeUndefined();
});

test.each([
['`Error` instance', new Error('Will I get caught twice?')],
['plain object', { 'Will I': 'get caught twice?' }],
Expand Down Expand Up @@ -1616,9 +1592,9 @@ describe('BaseClient', () => {

test('close', async () => {
jest.useRealTimers();
expect.assertions(2);
expect.assertions(4);

const { makeTransport, delay } = makeFakeTransport(300);
const { makeTransport, delay, getSentCount } = makeFakeTransport(300);

const client = new TestClient(
getDefaultTestClientOptions({
Expand All @@ -1630,9 +1606,12 @@ describe('BaseClient', () => {
expect(client.captureMessage('test')).toBeTruthy();

await client.close(delay);
expect(getSentCount()).toBe(1);

expect(client.captureMessage('test')).toBeTruthy();
await client.close(delay);
// Sends after close shouldn't work anymore
expect(client.captureMessage('test')).toBeFalsy();
expect(getSentCount()).toBe(1);
});

test('multiple concurrent flush calls should just work', async () => {
Expand Down Expand Up @@ -1798,18 +1777,6 @@ describe('BaseClient', () => {

expect(TestClient.instance!.session).toEqual(session);
});

test('skips when disabled', () => {
expect.assertions(1);

const options = getDefaultTestClientOptions({ enabled: false, dsn: PUBLIC_DSN });
const client = new TestClient(options);
const session = makeSession({ release: 'test' });

client.captureSession(session);

expect(TestClient.instance!.session).toBeUndefined();
});
});

describe('recordDroppedEvent()/_clearOutcomes()', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/sveltekit/test/server/handle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ describe('handleSentry', () => {
} catch (e) {
expect(mockCaptureException).toBeCalledTimes(1);
expect(addEventProcessorSpy).toBeCalledTimes(1);
expect(mockAddExceptionMechanism).toBeCalledTimes(1);
expect(mockAddExceptionMechanism).toBeCalledTimes(2);
expect(mockAddExceptionMechanism).toBeCalledWith(
{},
{ handled: false, type: 'sveltekit', data: { function: 'handle' } },
Expand Down
2 changes: 1 addition & 1 deletion packages/types/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ export interface Client<O extends ClientOptions = ClientOptions> {
addIntegration?(integration: Integration): void;

/** This is an internal function to setup all integrations that should run on the client */
setupIntegrations(): void;
setupIntegrations(forceInitialize?: boolean): void;

/** Creates an {@link Event} from all inputs to `captureException` and non-primitive inputs to `captureMessage`. */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down
4 changes: 2 additions & 2 deletions packages/utils/src/envelope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,14 +234,14 @@ export function createEventEnvelopeHeaders(
event: Event,
sdkInfo: SdkInfo | undefined,
tunnel: string | undefined,
dsn: DsnComponents,
dsn?: DsnComponents,
): EventEnvelopeHeaders {
const dynamicSamplingContext = event.sdkProcessingMetadata && event.sdkProcessingMetadata.dynamicSamplingContext;
return {
event_id: event.event_id as string,
sent_at: new Date().toISOString(),
...(sdkInfo && { sdk: sdkInfo }),
...(!!tunnel && { dsn: dsnToString(dsn) }),
...(!!tunnel && dsn && { dsn: dsnToString(dsn) }),
...(dynamicSamplingContext && {
trace: dropUndefinedKeys({ ...dynamicSamplingContext }),
}),
Expand Down