From d952992278e71e6ebeab5abf6857c8ee71faa785 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 16 Apr 2021 10:32:40 +0200 Subject: [PATCH 01/10] feat(node): Request mode sessions Captures request mode sessions associated with a specific release, and sends them to the Sentry as part of the Release Health functionality. --- packages/core/src/index.ts | 2 +- packages/core/src/request.ts | 21 +- packages/core/test/lib/request.test.ts | 51 ++++- packages/hub/src/index.ts | 2 +- packages/hub/src/scope.ts | 12 + packages/hub/src/session.ts | 132 ++++++++++- packages/hub/test/scope.test.ts | 30 ++- packages/hub/test/sessionflusher.test.ts | 128 +++++++++++ packages/node/package.json | 2 +- packages/node/src/client.ts | 93 +++++++- packages/node/src/handlers.ts | 71 +++++- packages/node/src/sdk.ts | 4 + packages/node/src/transports/http.ts | 11 +- packages/node/src/transports/https.ts | 11 +- packages/node/test/client.test.ts | 201 +++++++++++++++++ packages/node/test/handlers.test.ts | 213 +++++++++++++++++- .../aggregates-disable-single-session.js | 95 ++++++++ .../caught-exception-errored-session.js | 2 +- .../single-session/healthy-session.js | 2 +- .../uncaught-exception-crashed-session.js | 2 +- .../unhandled-rejection-crashed-session.js | 2 +- .../{single-session => }/test-utils.js | 0 packages/node/test/transports/http.test.ts | 28 ++- packages/node/test/transports/https.test.ts | 28 ++- packages/types/src/index.ts | 10 +- packages/types/src/scope.ts | 3 +- packages/types/src/session.ts | 44 ++++ packages/types/src/transport.ts | 9 +- 28 files changed, 1170 insertions(+), 39 deletions(-) create mode 100644 packages/hub/test/sessionflusher.test.ts create mode 100644 packages/node/test/client.test.ts create mode 100644 packages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js rename packages/node/test/manual/release-health/{single-session => }/test-utils.js (100%) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5fe8450de3ed..53848b1df5a5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -17,7 +17,7 @@ export { addGlobalEventProcessor, getCurrentHub, getHubFromCarrier, Hub, makeMai export { API } from './api'; export { BaseClient } from './baseclient'; export { BackendClass, BaseBackend } from './basebackend'; -export { eventToSentryRequest, sessionToSentryRequest } from './request'; +export { eventToSentryRequest, sessionToSentryRequest, sessionAggregatesToSentryRequest } from './request'; export { initAndBind, ClientClass } from './sdk'; export { NoopTransport } from './transports/noop'; export { SDK_VERSION } from './version'; diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts index 6cce3e539521..3ee62d64fd4c 100644 --- a/packages/core/src/request.ts +++ b/packages/core/src/request.ts @@ -1,4 +1,4 @@ -import { Event, SdkInfo, SentryRequest, Session } from '@sentry/types'; +import { Event, SdkInfo, SentryRequest, SentryRequestType, Session, SessionAggregates } from '@sentry/types'; import { API } from './api'; @@ -45,6 +45,25 @@ export function sessionToSentryRequest(session: Session, api: API): SentryReques }; } +/** Creates a SentryRequest from an Aggregates of Request mode sessions */ +export function sessionAggregatesToSentryRequest(sessionAggregates: SessionAggregates, api: API): SentryRequest { + const sdkInfo = getSdkMetadataForEnvelopeHeader(api); + const envelopeHeaders = JSON.stringify({ + sent_at: new Date().toISOString(), + ...(sdkInfo && { sdk: sdkInfo }), + }); + // The server expects type `sessions` in headers for Session Aggregates payload + const itemHeaders = JSON.stringify({ + type: 'sessions', + }); + + return { + body: `${envelopeHeaders}\n${itemHeaders}\n${JSON.stringify(sessionAggregates)}`, + type: 'sessions' as SentryRequestType, + url: api.getEnvelopeEndpointWithUrlEncodedAuth(), + }; +} + /** Creates a SentryRequest from an event. */ export function eventToSentryRequest(event: Event, api: API): SentryRequest { const sdkInfo = getSdkMetadataForEnvelopeHeader(api); diff --git a/packages/core/test/lib/request.test.ts b/packages/core/test/lib/request.test.ts index eb99d510dba5..076282bba998 100644 --- a/packages/core/test/lib/request.test.ts +++ b/packages/core/test/lib/request.test.ts @@ -1,9 +1,19 @@ import { DebugMeta, Event, SentryRequest, TransactionSamplingMethod } from '@sentry/types'; import { API } from '../../src/api'; -import { eventToSentryRequest } from '../../src/request'; +import { eventToSentryRequest, sessionAggregatesToSentryRequest } from '../../src/request'; + +const api = new API('https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', { + sdk: { + integrations: ['AWSLambda'], + name: 'sentry.javascript.browser', + version: `12.31.12`, + packages: [{ name: 'npm:@sentry/browser', version: `12.31.12` }], + }, +}); describe('eventToSentryRequest', () => { + let event: Event; function parseEnvelopeRequest(request: SentryRequest): any { const [envelopeHeaderString, itemHeaderString, eventString] = request.body.split('\n'); @@ -14,16 +24,6 @@ describe('eventToSentryRequest', () => { }; } - const api = new API('https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', { - sdk: { - integrations: ['AWSLambda'], - name: 'sentry.javascript.browser', - version: `12.31.12`, - packages: [{ name: 'npm:@sentry/browser', version: `12.31.12` }], - }, - }); - let event: Event; - beforeEach(() => { event = { contexts: { trace: { trace_id: '1231201211212012', span_id: '12261980', op: 'pageload' } }, @@ -125,3 +125,32 @@ describe('eventToSentryRequest', () => { ); }); }); + +describe('sessionAggregatesToSentryRequest', () => { + it('test envelope creation for aggregateSessions', () => { + const aggregatedSession = { + attrs: { release: '1.0.x', environment: 'prod' }, + aggregates: [{ started: '2021-04-08T12:18:00.000Z', exited: 2 }], + }; + const result = sessionAggregatesToSentryRequest(aggregatedSession, api); + + const [envelopeHeaderString, itemHeaderString, sessionString] = result.body.split('\n'); + + expect(JSON.parse(envelopeHeaderString)).toEqual( + expect.objectContaining({ + sdk: { name: 'sentry.javascript.browser', version: '12.31.12' }, + }), + ); + expect(JSON.parse(itemHeaderString)).toEqual( + expect.objectContaining({ + type: 'sessions', + }), + ); + expect(JSON.parse(sessionString)).toEqual( + expect.objectContaining({ + attrs: { release: '1.0.x', environment: 'prod' }, + aggregates: [{ started: '2021-04-08T12:18:00.000Z', exited: 2 }], + }), + ); + }); +}); diff --git a/packages/hub/src/index.ts b/packages/hub/src/index.ts index 56b2d666ad1f..b6be3f2744a7 100644 --- a/packages/hub/src/index.ts +++ b/packages/hub/src/index.ts @@ -1,7 +1,7 @@ // eslint-disable-next-line deprecation/deprecation export { Carrier, DomainAsCarrier, Layer } from './interfaces'; export { addGlobalEventProcessor, Scope } from './scope'; -export { Session } from './session'; +export { Session, SessionFlusher } from './session'; export { // eslint-disable-next-line deprecation/deprecation getActiveDomain, diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index 0ae4ffe3a8c0..a0c9e6e0dd70 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -10,6 +10,7 @@ import { Extra, Extras, Primitive, + RequestSessionStatus, Scope as ScopeInterface, ScopeContext, Severity, @@ -65,6 +66,9 @@ export class Scope implements ScopeInterface { /** Session */ protected _session?: Session; + /** Request Mode Session Status */ + protected _requestSession?: { status?: RequestSessionStatus }; + /** * Inherit values from the parent scope. * @param scope to clone. @@ -83,6 +87,7 @@ export class Scope implements ScopeInterface { newScope._transactionName = scope._transactionName; newScope._fingerprint = scope._fingerprint; newScope._eventProcessors = [...scope._eventProcessors]; + newScope._requestSession = scope._requestSession; } return newScope; } @@ -297,6 +302,9 @@ export class Scope implements ScopeInterface { if (captureContext._fingerprint) { this._fingerprint = captureContext._fingerprint; } + if (captureContext._requestSession) { + this._requestSession = captureContext._requestSession; + } } else if (isPlainObject(captureContext)) { // eslint-disable-next-line no-param-reassign captureContext = captureContext as ScopeContext; @@ -312,6 +320,9 @@ export class Scope implements ScopeInterface { if (captureContext.fingerprint) { this._fingerprint = captureContext.fingerprint; } + if (captureContext.requestSession) { + this._requestSession = captureContext.requestSession; + } } return this; @@ -329,6 +340,7 @@ export class Scope implements ScopeInterface { this._level = undefined; this._transactionName = undefined; this._fingerprint = undefined; + this._requestSession = undefined; this._span = undefined; this._session = undefined; this._notifyScopeListeners(); diff --git a/packages/hub/src/session.ts b/packages/hub/src/session.ts index e911a72077bf..c155d66a9fa4 100644 --- a/packages/hub/src/session.ts +++ b/packages/hub/src/session.ts @@ -1,5 +1,16 @@ -import { Session as SessionInterface, SessionContext, SessionStatus } from '@sentry/types'; -import { dropUndefinedKeys, uuid4 } from '@sentry/utils'; +import { + AggregationCounts, + RequestSessionStatus, + Session as SessionInterface, + SessionAggregates, + SessionContext, + SessionFlusher as SessionFlusherInterface, + SessionStatus, + Transport, +} from '@sentry/types'; +import { dropUndefinedKeys, logger, uuid4 } from '@sentry/utils'; + +import { getCurrentHub } from './hub'; /** * @inheritdoc @@ -123,3 +134,120 @@ export class Session implements SessionInterface { }); } } + +type releaseHealthAttributes = { + environment?: string; + release: string; +}; + +/** + * @inheritdoc + */ +export class SessionFlusher implements SessionFlusherInterface { + public readonly flushTimeout: number = 60; + private _pendingAggregates: { [key: number]: AggregationCounts } = {}; + private _sessionAttrs: releaseHealthAttributes; + private _intervalId: ReturnType; + private _isEnabled: boolean = true; + private _transport: Transport; + + constructor(transport: Transport, attrs: releaseHealthAttributes) { + this._transport = transport; + // Call to setInterval, so that flush is called every 60 seconds + this._intervalId = setInterval(() => this.flush(), this.flushTimeout * 1000); + this._sessionAttrs = attrs; + } + + /** Sends session aggregates to Transport */ + public sendSessionAggregates(sessionAggregates: SessionAggregates): void { + if (!this._transport.sendSessionAggregates) { + logger.warn("Dropping session because custom transport doesn't implement sendSessionAggregates"); + return; + } + this._transport.sendSessionAggregates(sessionAggregates).then(null, reason => { + logger.error(`Error while sending session: ${reason}`); + }); + } + + /** Checks if `pendingAggregates` has entries, and if it does flushes them by calling `sendSessions` */ + flush(): void { + const sessionAggregates = this.getSessionAggregates(); + if (sessionAggregates.aggregates.length === 0) { + return; + } + this._pendingAggregates = {}; + this.sendSessionAggregates(sessionAggregates); + } + + /** Massages the entries in `pendingAggregates` and returns aggregated sessions */ + getSessionAggregates(): SessionAggregates { + const aggregates: AggregationCounts[] = Object.keys(this._pendingAggregates).map((key: string) => { + return this._pendingAggregates[parseInt(key)]; + }); + + const sessionAggregates: SessionAggregates = { + attrs: this._sessionAttrs, + aggregates: aggregates, + }; + return dropUndefinedKeys(sessionAggregates); + } + + /** JSDoc */ + close(): void { + clearInterval(this._intervalId); + this._isEnabled = false; + this.flush(); + } + + /** + * Wrapper function for _incrementSessionStatusCount that checks if the instance of SessionFlusher is enabled then + * fetches the session status of the request from `_requestSession.status` on the scope and passes them to + * `_incrementSessionStatusCount` along with the start date + */ + public incrementSessionStatusCount(): void { + if (!this._isEnabled) { + return; + } + const scope = getCurrentHub().getScope(); + /* eslint-disable @typescript-eslint/no-unsafe-member-access */ + const requestSession = (scope as any)._requestSession; + + if (requestSession && requestSession.status) { + this._incrementSessionStatusCount(requestSession.status, new Date()); + // This is not entirely necessarily but is added as a safe guard to indicate the bounds of a request and so in + // case captureRequestSession is called more than once to prevent double count + (scope as any)._requestSession = undefined; + + /* eslint-enable @typescript-eslint/no-unsafe-member-access */ + } + } + + /** + * Increments status bucket in pendingAggregates buffer (internal state) corresponding to status of + * the session received + */ + private _incrementSessionStatusCount(status: RequestSessionStatus, date: Date): number { + // Truncate minutes and seconds on Session Started attribute to have one minute bucket keys + const sessionStartedTrunc: number = new Date(date).setSeconds(0, 0); + this._pendingAggregates[sessionStartedTrunc] = this._pendingAggregates[sessionStartedTrunc] || {}; + + // corresponds to aggregated sessions in one specific minute bucket + // for example, {"started":"2021-03-16T08:00:00.000Z","exited":4, "errored": 1} + const aggregationCounts: AggregationCounts = this._pendingAggregates[sessionStartedTrunc]; + if (!aggregationCounts.started) { + aggregationCounts.started = new Date(sessionStartedTrunc).toISOString(); + } + + switch (status) { + case RequestSessionStatus.Errored: + aggregationCounts.errored = aggregationCounts.errored !== undefined ? aggregationCounts.errored + 1 : 1; + return aggregationCounts.errored; + case RequestSessionStatus.Ok: + aggregationCounts.exited = aggregationCounts.exited !== undefined ? aggregationCounts.exited + 1 : 1; + return aggregationCounts.exited; + case RequestSessionStatus.Crashed: + aggregationCounts.crashed = aggregationCounts.crashed !== undefined ? aggregationCounts.crashed + 1 : 1; + return aggregationCounts.crashed; + } + } +} diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts index a7b512aa375f..9c405e8091e8 100644 --- a/packages/hub/test/scope.test.ts +++ b/packages/hub/test/scope.test.ts @@ -1,4 +1,4 @@ -import { Event, EventHint, Severity } from '@sentry/types'; +import { Event, EventHint, RequestSessionStatus, Severity } from '@sentry/types'; import { getGlobalObject } from '@sentry/utils'; import { addGlobalEventProcessor, Scope } from '../src'; @@ -129,6 +129,13 @@ describe('Scope', () => { expect((parentScope as any)._extra).toEqual((scope as any)._extra); }); + test('_requestSession clone', () => { + const parentScope = new Scope(); + (parentScope as any)._requestSession = { status: RequestSessionStatus.Errored }; + const scope = Scope.clone(parentScope); + expect((parentScope as any)._requestSession).toEqual((scope as any)._requestSession); + }); + test('parent changed inheritance', () => { const parentScope = new Scope(); const scope = Scope.clone(parentScope); @@ -146,6 +153,19 @@ describe('Scope', () => { expect((parentScope as any)._extra).toEqual({ a: 1 }); expect((scope as any)._extra).toEqual({ a: 2 }); }); + + test('child override should set the value of parent _requestSession', () => { + // Test that ensures if the status value of `status` of `_requestSession` is changed in a child scope + // that it should also change in parent scope because we are copying the reference to the object + const parentScope = new Scope(); + (parentScope as any)._requestSession = { status: RequestSessionStatus.Errored }; + + const scope = Scope.clone(parentScope); + (scope as any)._requestSession.status = RequestSessionStatus.Ok; + + expect((parentScope as any)._requestSession).toEqual({ status: RequestSessionStatus.Ok }); + expect((scope as any)._requestSession).toEqual({ status: RequestSessionStatus.Ok }); + }); }); describe('applyToEvent', () => { @@ -336,9 +356,11 @@ describe('Scope', () => { scope.setUser({ id: '1' }); scope.setFingerprint(['abcd']); scope.addBreadcrumb({ message: 'test' }, 100); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; expect((scope as any)._extra).toEqual({ a: 2 }); scope.clear(); expect((scope as any)._extra).toEqual({}); + expect((scope as any)._requestSession).toEqual(undefined); }); test('clearBreadcrumbs', () => { @@ -361,6 +383,7 @@ describe('Scope', () => { scope.setUser({ id: '1337' }); scope.setLevel(Severity.Info); scope.setFingerprint(['foo']); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; }); test('given no data, returns the original scope', () => { @@ -408,6 +431,7 @@ describe('Scope', () => { localScope.setUser({ id: '42' }); localScope.setLevel(Severity.Warning); localScope.setFingerprint(['bar']); + (localScope as any)._requestSession = { status: RequestSessionStatus.Ok }; const updatedScope = scope.update(localScope) as any; @@ -429,6 +453,7 @@ describe('Scope', () => { expect(updatedScope._user).toEqual({ id: '42' }); expect(updatedScope._level).toEqual(Severity.Warning); expect(updatedScope._fingerprint).toEqual(['bar']); + expect(updatedScope._requestSession.status).toEqual(RequestSessionStatus.Ok); }); test('given an empty instance of Scope, it should preserve all the original scope data', () => { @@ -449,6 +474,7 @@ describe('Scope', () => { expect(updatedScope._user).toEqual({ id: '1337' }); expect(updatedScope._level).toEqual(Severity.Info); expect(updatedScope._fingerprint).toEqual(['foo']); + expect(updatedScope._requestSession.status).toEqual(RequestSessionStatus.Ok); }); test('given a plain object, it should merge two together, with the passed object having priority', () => { @@ -459,6 +485,7 @@ describe('Scope', () => { level: Severity.Warning, tags: { bar: '3', baz: '4' }, user: { id: '42' }, + requestSession: { status: RequestSessionStatus.Errored }, }; const updatedScope = scope.update(localAttributes) as any; @@ -480,6 +507,7 @@ describe('Scope', () => { expect(updatedScope._user).toEqual({ id: '42' }); expect(updatedScope._level).toEqual(Severity.Warning); expect(updatedScope._fingerprint).toEqual(['bar']); + expect(updatedScope._requestSession).toEqual({ status: RequestSessionStatus.Errored }); }); }); diff --git a/packages/hub/test/sessionflusher.test.ts b/packages/hub/test/sessionflusher.test.ts new file mode 100644 index 000000000000..7f3ae2cd776b --- /dev/null +++ b/packages/hub/test/sessionflusher.test.ts @@ -0,0 +1,128 @@ +import { RequestSessionStatus, Status } from '@sentry/types'; + +import { SessionFlusher } from '../src'; + +describe('Session Flusher', () => { + let sendSessionAggregates: jest.Mock; + let transport: { + sendEvent: jest.Mock; + sendSessionAggregates: jest.Mock; + close: jest.Mock; + }; + + beforeEach(() => { + jest.useFakeTimers(); + sendSessionAggregates = jest.fn(() => Promise.resolve({ status: Status.Success })); + transport = { + sendEvent: jest.fn(), + sendSessionAggregates, + close: jest.fn(), + }; + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + test('test incrementSessionStatusCount updates the internal SessionFlusher state', () => { + const flusher = new SessionFlusher(transport, { release: '1.0.0', environment: 'dev' }); + + const date = new Date('2021-04-08T12:18:23.043Z'); + let count = (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); + expect(count).toEqual(1); + count = (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); + expect(count).toEqual(2); + count = (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Errored, date); + expect(count).toEqual(1); + date.setMinutes(date.getMinutes() + 1); + count = (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); + expect(count).toEqual(1); + count = (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Errored, date); + expect(count).toEqual(1); + + expect(flusher.getSessionAggregates().aggregates).toEqual([ + { errored: 1, exited: 2, started: '2021-04-08T12:18:00.000Z' }, + { errored: 1, exited: 1, started: '2021-04-08T12:19:00.000Z' }, + ]); + expect(flusher.getSessionAggregates().attrs).toEqual({ release: '1.0.0', environment: 'dev' }); + }); + + test('test undefined attributes are excluded, on incrementSessionStatusCount call', () => { + const flusher = new SessionFlusher(transport, { release: '1.0.0' }); + + const date = new Date('2021-04-08T12:18:23.043Z'); + (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); + (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Errored, date); + + expect(flusher.getSessionAggregates()).toEqual({ + aggregates: [{ errored: 1, exited: 1, started: '2021-04-08T12:18:00.000Z' }], + attrs: { release: '1.0.0' }, + }); + }); + + test('flush is called every 60 seconds after initialisation of an instance of SessionFlusher', () => { + const flusher = new SessionFlusher(transport, { release: '1.0.0', environment: 'dev' }); + const flusherFlushFunc = jest.spyOn(flusher, 'flush'); + jest.advanceTimersByTime(59000); + expect(flusherFlushFunc).toHaveBeenCalledTimes(0); + jest.advanceTimersByTime(2000); + expect(flusherFlushFunc).toHaveBeenCalledTimes(1); + jest.advanceTimersByTime(58000); + expect(flusherFlushFunc).toHaveBeenCalledTimes(1); + jest.advanceTimersByTime(2000); + expect(flusherFlushFunc).toHaveBeenCalledTimes(2); + }); + + test('sendSessions is called on flush if sessions were captured', () => { + const flusher = new SessionFlusher(transport, { release: '1.0.0', environment: 'dev' }); + const flusherFlushFunc = jest.spyOn(flusher, 'flush'); + const date = new Date('2021-04-08T12:18:23.043Z'); + (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); + (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); + + expect(sendSessionAggregates).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(61000); + + expect(flusherFlushFunc).toHaveBeenCalledTimes(1); + expect(sendSessionAggregates).toHaveBeenCalledWith( + expect.objectContaining({ + attrs: { release: '1.0.0', environment: 'dev' }, + aggregates: [{ started: '2021-04-08T12:18:00.000Z', exited: 2 }], + }), + ); + }); + + test('sendSessions is not called on flush if no sessions were captured', () => { + const flusher = new SessionFlusher(transport, { release: '1.0.0', environment: 'dev' }); + const flusherFlushFunc = jest.spyOn(flusher, 'flush'); + + expect(sendSessionAggregates).toHaveBeenCalledTimes(0); + jest.advanceTimersByTime(61000); + expect(flusherFlushFunc).toHaveBeenCalledTimes(1); + expect(sendSessionAggregates).toHaveBeenCalledTimes(0); + }); + + test('calling close on SessionFlusher should disable SessionFlusher', () => { + const flusher = new SessionFlusher(transport, { release: '1.0.x' }); + flusher.close(); + expect((flusher as any)._isEnabled).toEqual(false); + }); + + test('calling close on SessionFlusher will force call flush', () => { + const flusher = new SessionFlusher(transport, { release: '1.0.x' }); + const flusherFlushFunc = jest.spyOn(flusher, 'flush'); + const date = new Date('2021-04-08T12:18:23.043Z'); + (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); + (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); + flusher.close(); + + expect(flusherFlushFunc).toHaveBeenCalledTimes(1); + expect(sendSessionAggregates).toHaveBeenCalledWith( + expect.objectContaining({ + attrs: { release: '1.0.x' }, + aggregates: [{ started: '2021-04-08T12:18:00.000Z', exited: 2 }], + }), + ); + }); +}); diff --git a/packages/node/package.json b/packages/node/package.json index c3ad0cd6508b..9bf06e83cbd5 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -61,7 +61,7 @@ "test:watch": "jest --watch", "test:express": "node test/manual/express-scope-separation/start.js", "test:webpack": "cd test/manual/webpack-domain/ && yarn && node npm-build.js", - "test:release-health": "node test/manual/release-health/single-session/healthy-session.js && node test/manual/release-health/single-session/caught-exception-errored-session.js && node test/manual/release-health/single-session/uncaught-exception-crashed-session.js && node test/manual/release-health/single-session/unhandled-rejection-crashed-session.js", + "test:release-health": "node test/manual/release-health/single-session/healthy-session.js && node test/manual/release-health/single-session/caught-exception-errored-session.js && node test/manual/release-health/single-session/uncaught-exception-crashed-session.js && node test/manual/release-health/single-session/unhandled-rejection-crashed-session.js && node test/manual/release-health/session-aggregates/aggregates-disable-single-session.js", "pack": "npm pack" }, "volta": { diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index ed6ffd74bf89..c68278cbe684 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -1,5 +1,7 @@ import { BaseClient, Scope, SDK_VERSION } from '@sentry/core'; -import { Event, EventHint } from '@sentry/types'; +import { SessionFlusher } from '@sentry/hub'; +import { Event, EventHint, RequestSessionStatus } from '@sentry/types'; +import { logger } from '@sentry/utils'; import { NodeBackend, NodeOptions } from './backend'; @@ -10,6 +12,7 @@ import { NodeBackend, NodeOptions } from './backend'; * @see SentryClient for usage documentation. */ export class NodeClient extends BaseClient { + protected _sessionFlusher: SessionFlusher | undefined; /** * Creates a new Node SDK instance. * @param options Configuration options for this SDK. @@ -30,6 +33,69 @@ export class NodeClient extends BaseClient { super(NodeBackend, options); } + /** + * @inheritDoc + */ + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types + public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined { + /* eslint-disable @typescript-eslint/no-unsafe-member-access */ + + // Check if the flag `autoSessionTracking` is enabled, and if `_sessionFlusher` exists because it is initialised only + // when the `requestHandler` middleware is used, and hence the expectation is to have SessionAggregates payload + // sent to the Server only when the `requestHandler` middleware is used + if (this._options.autoSessionTracking && this._sessionFlusher && scope) { + const requestSession = (scope as any)._requestSession; + + // Necessary checks to ensure this is code block is executed only within a request + // Should override the status only if `requestSession.status` is `Ok`, which is its initial stage + if (requestSession && requestSession.status === RequestSessionStatus.Ok) { + requestSession.status = RequestSessionStatus.Errored; + } + } + /* eslint-enable @typescript-eslint/no-unsafe-member-access */ + + return super.captureException(exception, hint, scope); + } + + /** + * @inheritDoc + */ + public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined { + // Check if the flag `autoSessionTracking` is enabled, and if `_sessionFlusher` exists because it is initialised only + // when the `requestHandler` middleware is used, and hence the expectation is to have SessionAggregates payload + // sent to the Server only when the `requestHandler` middleware is used + if (this._options.autoSessionTracking && this._sessionFlusher && scope) { + const eventType = event.type || 'exception'; + const isException = + eventType === 'exception' && event.exception && event.exception.values && event.exception.values.length > 0; + + // If the event is of type Exception, then a request session should be captured + if (isException) { + /* eslint-disable @typescript-eslint/no-unsafe-member-access */ + const requestSession = (scope as any)._requestSession; + + // Ensure that this is happening within the bounds of a request, and make sure not to override + // Session Status if Errored / Crashed + if (requestSession && requestSession.status === RequestSessionStatus.Ok) { + (scope as any)._requestSession.status = RequestSessionStatus.Errored; + } + + /* eslint-enable @typescript-eslint/no-unsafe-member-access */ + } + } + + return super.captureEvent(event, hint, scope); + } + + /** + * + * @inheritdoc + */ + public close(timeout?: number): PromiseLike { + this._sessionFlusher?.close(); + return super.close(timeout); + } + /** * @inheritDoc */ @@ -40,4 +106,29 @@ export class NodeClient extends BaseClient { } return super._prepareEvent(event, scope, hint); } + + /** Method that initialises an instance of SessionFlusher on Client */ + protected _initSessionFlusher(): void { + const { release, environment } = this._options; + if (!release) { + logger.warn('Cannot initialise an instance of SessionFlusher if no release is provided!'); + } else { + this._sessionFlusher = new SessionFlusher(this._backend.getTransport(), { + release, + environment, + }); + } + } + + /** + * Method responsible for capturing/ending a request session by calling `incrementSessionStatusCount` to increment + * appropriate session aggregates bucket + */ + protected _captureRequestSession(): void { + if (!this._sessionFlusher) { + logger.warn('Discarded request mode session because autoSessionTracking option was disabled'); + } else { + this._sessionFlusher.incrementSessionStatusCount(); + } + } } diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 7496357851c2..bff33a252ae3 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -2,7 +2,7 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core'; import { extractTraceparentData, Span } from '@sentry/tracing'; -import { Event, ExtractedNodeRequestData, Transaction } from '@sentry/types'; +import { Event, ExtractedNodeRequestData, RequestSessionStatus, Transaction } from '@sentry/types'; import { isPlainObject, isString, logger, normalize, stripUrlQueryAndFragment } from '@sentry/utils'; import * as cookie from 'cookie'; import * as domain from 'domain'; @@ -10,7 +10,8 @@ import * as http from 'http'; import * as os from 'os'; import * as url from 'url'; -import { flush } from './sdk'; +import { NodeClient } from './client'; +import { flush, isAutoSessionTrackingEnabled } from './sdk'; export interface ExpressRequest { baseUrl?: string; @@ -370,6 +371,20 @@ export type RequestHandlerOptions = ParseRequestOptions & { export function requestHandler( options?: RequestHandlerOptions, ): (req: http.IncomingMessage, res: http.ServerResponse, next: (error?: any) => void) => void { + const currentHub = getCurrentHub(); + const client = currentHub.getClient(); + // Initialise an instance of SessionFlusher on the client when `autoSessionTracking` is enabled and the + // `requestHandler` middleware is used indicating that we are running in SessionAggregates mode + if (client && isAutoSessionTrackingEnabled(client)) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + (client as any)._initSessionFlusher(); + + // If Scope contains a Single mode Session, it is removed in favor of using Session Aggregates mode + const scope = currentHub.getScope(); + if (scope && scope.getSession()) { + scope.setSession(); + } + } return function sentryRequestMiddleware( req: http.IncomingMessage, res: http.ServerResponse, @@ -392,10 +407,36 @@ export function requestHandler( local.add(req); local.add(res); local.on('error', next); + local.run(() => { - getCurrentHub().configureScope(scope => - scope.addEventProcessor((event: Event) => parseRequest(event, req, options)), - ); + const currentHub = getCurrentHub(); + + currentHub.configureScope(scope => { + scope.addEventProcessor((event: Event) => parseRequest(event, req, options)); + const client = currentHub.getClient(); + if (isAutoSessionTrackingEnabled(client)) { + const scope = currentHub.getScope(); + if (scope) { + // Set `status` of `_requestSession` to Ok, at the beginning of the request + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + } + } + }); + + res.once('finish', () => { + const client = currentHub.getClient(); + if (isAutoSessionTrackingEnabled(client)) { + setImmediate(() => { + if (client) { + // Calling _captureRequestSession to capture request session at the end of the request by incrementing + // the correct SessionAggregates bucket i.e. crashed, errored or exited + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + (client as any)._captureRequestSession(); + } + }); + } + }); next(); }); }; @@ -456,6 +497,26 @@ export function errorHandler(options?: { if (transaction && _scope.getSpan() === undefined) { _scope.setSpan(transaction); } + + const client = getCurrentHub().getClient(); + if (client && isAutoSessionTrackingEnabled(client)) { + /* eslint-disable @typescript-eslint/no-unsafe-member-access */ + // Check if the `SessionFlusher` is instantiated on the client to go into this branch that marks the + // `requestSession.status` as `Crashed`, and this check is necessary because the `SessionFlusher` is only + // instantiated when the the`requestHandler` middleware is initialised, which indicates that we should be + // running in SessionAggregates mode + const isSessionAggregatesMode = (client as any)._sessionFlusher !== undefined; + if (isSessionAggregatesMode) { + const requestSession = (_scope as any)._requestSession; + // If an error bubbles to the `errorHandler`, then this is an unhandled error, and should be reported as a + // Crashed session. The `_requestSession.status` is checked to ensure that this error is happening within + // the bounds of a request, and if so the status is updated + if (requestSession && requestSession.status !== undefined) + requestSession.status = RequestSessionStatus.Crashed; + } + } + /* eslint-enable @typescript-eslint/no-unsafe-member-access */ + const eventId = captureException(error); // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access (res as any).sentry = eventId; diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index ccd40d9510ae..ad88b931c616 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -118,6 +118,10 @@ export function init(options: NodeOptions = {}): void { options.environment = process.env.SENTRY_ENVIRONMENT; } + if (options.autoSessionTracking === undefined) { + options.autoSessionTracking = true; + } + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any if ((domain as any).active) { setHubOnCarrier(carrier, getCurrentHub()); diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts index c4de15073f7a..5a4dff8a9bd5 100644 --- a/packages/node/src/transports/http.ts +++ b/packages/node/src/transports/http.ts @@ -1,5 +1,5 @@ -import { eventToSentryRequest, sessionToSentryRequest } from '@sentry/core'; -import { Event, Response, Session, TransportOptions } from '@sentry/types'; +import { eventToSentryRequest, sessionAggregatesToSentryRequest, sessionToSentryRequest } from '@sentry/core'; +import { Event, Response, Session, SessionAggregates, TransportOptions } from '@sentry/types'; import * as http from 'http'; import { BaseTransport } from './base'; @@ -29,4 +29,11 @@ export class HTTPTransport extends BaseTransport { public sendSession(session: Session): PromiseLike { return this._send(sessionToSentryRequest(session, this._api), session); } + + /** + * @inheritDoc + */ + public sendSessionAggregates(sessionAggregates: SessionAggregates): PromiseLike { + return this._send(sessionAggregatesToSentryRequest(sessionAggregates, this._api)); + } } diff --git a/packages/node/src/transports/https.ts b/packages/node/src/transports/https.ts index 96499504a2fa..a23352abc891 100644 --- a/packages/node/src/transports/https.ts +++ b/packages/node/src/transports/https.ts @@ -1,5 +1,5 @@ -import { eventToSentryRequest, sessionToSentryRequest } from '@sentry/core'; -import { Event, Response, Session, TransportOptions } from '@sentry/types'; +import { eventToSentryRequest, sessionAggregatesToSentryRequest, sessionToSentryRequest } from '@sentry/core'; +import { Event, Response, Session, SessionAggregates, TransportOptions } from '@sentry/types'; import * as https from 'https'; import { BaseTransport } from './base'; @@ -29,4 +29,11 @@ export class HTTPSTransport extends BaseTransport { public sendSession(session: Session): PromiseLike { return this._send(sessionToSentryRequest(session, this._api), session); } + + /** + * @inheritDoc + */ + public sendSessionAggregates(sessionAggregates: SessionAggregates): PromiseLike { + return this._send(sessionAggregatesToSentryRequest(sessionAggregates, this._api)); + } } diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts new file mode 100644 index 000000000000..3ad1e5eeeef0 --- /dev/null +++ b/packages/node/test/client.test.ts @@ -0,0 +1,201 @@ +import { Scope, SessionFlusher } from '@sentry/hub'; +import { RequestSessionStatus } from '@sentry/types'; + +import { NodeClient } from '../src'; + +const PUBLIC_DSN = 'https://username@domain/123'; + +describe('NodeClient', () => { + let client: NodeClient; + + afterEach(() => { + if ('_sessionFlusher' in client) clearInterval((client as any)._sessionFlusher._intervalId); + jest.restoreAllMocks(); + }); + + describe('captureException', () => { + test('when autoSessionTracking is enabled, and requestHandler is not used -> requestStatus should not be set', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + + client.captureException(new Error('test exception'), undefined, scope); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + }); + test('when autoSessionTracking is disabled -> requestStatus should not be set', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + + client.captureException(new Error('test exception'), undefined, scope); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + }); + test('when autoSessionTracking is enabled + requestSession status is Crashed -> requestStatus should not be overridden', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Crashed }; + + client.captureException(new Error('test exception'), undefined, scope); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Crashed); + }); + test('when autoSessionTracking is enabled + error occurs within request bounds -> requestStatus should be set to Errored', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + + client.captureException(new Error('test exception'), undefined, scope); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Errored); + }); + test('when autoSessionTracking is enabled + error occurs outside of request bounds -> requestStatus should not be set to Errored', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + + client.captureException(new Error('test exception'), undefined, scope); + + const requestSession = (scope as any)._requestSession; + expect(requestSession).toEqual(undefined); + }); + }); + + describe('captureEvent()', () => { + test('If autoSessionTracking is disabled, requestSession status should not be set', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + client.captureEvent( + { message: 'message', exception: { values: [{ type: 'exception type 1' }] } }, + undefined, + scope, + ); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + }); + + test('When captureEvent is called with an exception, requestSession status should be set to Errored', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + + client.captureEvent({ message: 'message', exception: { values: [{ type: 'exception type 1' }] } }, {}, scope); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Errored); + }); + + test('When captureEvent is called without an exception, requestSession status should not be set to Errored', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + + client.captureEvent({ message: 'message' }, {}, scope); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + }); + + test('When captureEvent is called with an exception but outside of a request, then requestStatus should not be set', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + + client.captureEvent( + { message: 'message', exception: { values: [{ type: 'exception type 1' }] } }, + undefined, + scope, + ); + + expect((scope as any)._requestSession).toEqual(undefined); + }); + + test('When captureEvent is called with a transaction, then requestSession status should not be set', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.3' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + client.captureEvent({ message: 'message', type: 'transaction' }, undefined, scope); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + }); + + test('When captureEvent is called with an exception but requestHandler is not used, then requestSession status should not be set', () => { + client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.3' }); + + const scope = new Scope(); + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + client.captureEvent( + { message: 'message', exception: { values: [{ type: 'exception type 1' }] } }, + undefined, + scope, + ); + + const requestSession = (scope as any)._requestSession; + expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + }); + }); +}); + +describe('flush/close', () => { + test('client close function disables _sessionFlusher', async () => { + jest.useRealTimers(); + const client = new NodeClient({ + dsn: PUBLIC_DSN, + autoSessionTracking: true, + release: '1.1', + }); + (client as any)._initSessionFlusher(); + // Clearing interval is important here to ensure that the flush function later on is called by the `client.close()` + // not due to the interval running every 60s + clearInterval((client as any)._sessionFlusher._intervalId); + + const sessionFlusherFlushFunc = jest.spyOn(SessionFlusher.prototype, 'flush'); + + const delay = 1; + await client.close(delay); + expect((client as any)._sessionFlusher._isEnabled).toBeFalsy(); + expect(sessionFlusherFlushFunc).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index af3ee7031ef3..27df4f308e0e 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -2,13 +2,20 @@ import * as sentryCore from '@sentry/core'; import { Hub } from '@sentry/hub'; import * as sentryHub from '@sentry/hub'; import { SpanStatus, Transaction } from '@sentry/tracing'; -import { Runtime } from '@sentry/types'; +import { RequestSessionStatus, Runtime } from '@sentry/types'; import * as http from 'http'; import * as net from 'net'; import { Event, Request, User } from '../src'; import { NodeClient } from '../src/client'; -import { ExpressRequest, extractRequestData, parseRequest, tracingHandler } from '../src/handlers'; +import { + errorHandler, + ExpressRequest, + extractRequestData, + parseRequest, + requestHandler, + tracingHandler, +} from '../src/handlers'; describe('parseRequest', () => { let mockReq: { [key: string]: any }; @@ -179,6 +186,103 @@ describe('parseRequest', () => { }); }); +describe('requestHandler', () => { + const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; + const method = 'wagging'; + const protocol = 'mutualsniffing'; + const hostname = 'the.dog.park'; + const path = '/by/the/trees/'; + const queryString = 'chase=me&please=thankyou'; + + const sentryRequestMiddleware = requestHandler(); + + let req: http.IncomingMessage, res: http.ServerResponse, next: () => undefined; + let client: NodeClient; + + function createNoOpSpy() { + const noop = { noop: () => undefined }; // this is wrapped in an object so jest can spy on it + return jest.spyOn(noop, 'noop') as any; + } + + beforeEach(() => { + req = ({ + headers, + method, + protocol, + hostname, + originalUrl: `${path}?${queryString}`, + } as unknown) as http.IncomingMessage; + res = new http.ServerResponse(req); + next = createNoOpSpy(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('autoSessionTracking is enabled, sets requestSession status to ok, when handling a request', () => { + client = new NodeClient({ autoSessionTracking: true, release: '1.2' }); + const hub = new Hub(client); + + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + + sentryRequestMiddleware(req, res, next); + + const scope = sentryCore.getCurrentHub().getScope(); + expect((scope as any)._requestSession).toEqual({ status: RequestSessionStatus.Ok }); + }); + + it('autoSessionTracking is disabled, does not set requestSession, when handling a request', () => { + client = new NodeClient({ autoSessionTracking: false, release: '1.2' }); + const hub = new Hub(client); + + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + + sentryRequestMiddleware(req, res, next); + + const scope = sentryCore.getCurrentHub().getScope(); + expect((scope as any)._requestSession).toBeUndefined(); + }); + + it('autoSessionTracking is enabled, calls _captureRequestSession, on response finish', done => { + client = new NodeClient({ autoSessionTracking: true, release: '1.2' }); + const hub = new Hub(client); + + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + + const captureRequestSession = jest.spyOn(client, '_captureRequestSession'); + + sentryRequestMiddleware(req, res, next); + + const scope = sentryCore.getCurrentHub().getScope(); + res.emit('finish'); + + setImmediate(() => { + expect((scope as any)._requestSession).toEqual({ status: RequestSessionStatus.Ok }); + expect(captureRequestSession).toHaveBeenCalled(); + done(); + }); + }); + + it('autoSessionTracking is disabled, does not call _captureRequestSession, on response finish', done => { + client = new NodeClient({ autoSessionTracking: false, release: '1.2' }); + const hub = new Hub(client); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + + const captureRequestSession = jest.spyOn(client, '_captureRequestSession'); + + sentryRequestMiddleware(req, res, next); + const scope = sentryCore.getCurrentHub().getScope(); + res.emit('finish'); + + setImmediate(() => { + expect((scope as any)._requestSession).toBeUndefined(); + expect(captureRequestSession).not.toHaveBeenCalled(); + done(); + }); + }); +}); + describe('tracingHandler', () => { const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; const method = 'wagging'; @@ -556,3 +660,108 @@ describe('extractRequestData()', () => { }); }); }); + +describe('errorHandler()', () => { + const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; + const method = 'wagging'; + const protocol = 'mutualsniffing'; + const hostname = 'the.dog.park'; + const path = '/by/the/trees/'; + const queryString = 'chase=me&please=thankyou'; + + const sentryErrorMiddleware = errorHandler(); + + let req: http.IncomingMessage, res: http.ServerResponse, next: () => undefined; + let client: NodeClient; + + function createNoOpSpy() { + const noop = { noop: () => undefined }; // this is wrapped in an object so jest can spy on it + return jest.spyOn(noop, 'noop') as any; + } + + beforeEach(() => { + req = ({ + headers, + method, + protocol, + hostname, + originalUrl: `${path}?${queryString}`, + } as unknown) as http.IncomingMessage; + res = new http.ServerResponse(req); + next = createNoOpSpy(); + }); + + afterEach(() => { + if ('_sessionFlusher' in client) clearInterval((client as any)._sessionFlusher._intervalId); + jest.restoreAllMocks(); + }); + it('when autoSessionTracking is disabled, does not set requestSession status on Crash', () => { + client = new NodeClient({ autoSessionTracking: false, release: '3.3' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + + const scope = sentryCore.getCurrentHub().getScope(); + const hub = new Hub(client); + + jest.spyOn(client, '_captureRequestSession'); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub); + + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next); + const requestSession = (scope as any)._requestSession; + expect(requestSession).toEqual({ status: RequestSessionStatus.Ok }); + }); + + it('autoSessionTracking is enabled + requestHandler is not used -> does not set requestSession status on Crash', () => { + client = new NodeClient({ autoSessionTracking: false, release: '3.3' }); + + const scope = sentryCore.getCurrentHub().getScope(); + const hub = new Hub(client); + + jest.spyOn(client, '_captureRequestSession'); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub); + + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next); + const requestSession = (scope as any)._requestSession; + expect(requestSession).toEqual({ status: RequestSessionStatus.Ok }); + }); + + it('when autoSessionTracking is enabled, should set requestSession status to Crashed when an unhandled error occurs within the bounds of a request', () => { + client = new NodeClient({ autoSessionTracking: true, release: '1.1' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + const scope = new sentryHub.Scope(); + const hub = new Hub(client, scope); + + jest.spyOn(client, '_captureRequestSession'); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub); + + (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next); + const requestSession = (scope as any)._requestSession; + expect(requestSession).toEqual({ status: RequestSessionStatus.Crashed }); + }); + + it('when autoSessionTracking is enabled, should not set requestSession status on Crash when it occurs outside the bounds of a request', () => { + client = new NodeClient({ autoSessionTracking: true, release: '2.2' }); + // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised + // by the`requestHandler`) + (client as any)._initSessionFlusher(); + const scope = new sentryHub.Scope(); + const hub = new Hub(client, scope); + + jest.spyOn(client, '_captureRequestSession'); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); + jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub); + + sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next); + const requestSession = (scope as any)._requestSession; + expect(requestSession).toEqual(undefined); + }); +}); diff --git a/packages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js b/packages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js new file mode 100644 index 000000000000..6d93f43f74ea --- /dev/null +++ b/packages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js @@ -0,0 +1,95 @@ +const http = require('http'); +const express = require('express'); +const app = express(); +const Sentry = require('../../../../dist'); +const { assertSessions, BaseDummyTransport } = require('../test-utils'); + +function cleanUpAndExitSuccessfully() { + console.log('SUCCESS: Session Aggregates payload is correct!'); + server.close(); + clearInterval(flusher._intervalId) + process.exit(0); +} + +function assertSessionAggregates(session, expected) { + // For loop is added here just in the rare occasion that the session count do not land in the same aggregate + // bucket + session.aggregates.forEach(function (_, idx) { + delete session.aggregates[idx].started; + // Session Aggregates keys need to be ordered for JSON.stringify comparison + const ordered = Object.keys(session.aggregates[idx]).sort().reduce( + (obj, key) => { + obj[key] = session.aggregates[idx][key]; + return obj; + }, + {} + ); + session.aggregates[idx] = ordered; + }) + assertSessions(session, expected) +} + +class DummyTransport extends BaseDummyTransport { + sendSession(session) { + console.error('FAIL: Received Single Session which should have been deleted in favor of Session Aggregates!'); + process.exit(1); + } + sendSessionAggregates(session) { + assertSessionAggregates(session, {"attrs":{"release":"1.1"},"aggregates":[{"crashed":2,"errored":1,"exited":1}]}) + + cleanUpAndExitSuccessfully(); + + return Promise.resolve({ + status: 'success', + }); + } +} + +Sentry.init({ + dsn: 'http://test@example.com/1337', + release: '1.1', + transport: DummyTransport, + autoSessionTracking: true, +}); +/** + * Test that ensures that when `autoSessionTracking` is enabled and the express `requestHandler` middleware is used + * then Single Session should be disabled in favor of sending SessionAggregates. + */ + +app.use(Sentry.Handlers.requestHandler()); + +// Hack that resets the 60s default flush interval, and replaces it with just a one second interval +const flusher = Sentry.getCurrentHub().getClient()._sessionFlusher; +clearInterval(flusher._intervalId); +flusher._intervalId = setInterval(() => flusher.flush(), 1000); + + +app.get('/foo', (req, res, next) => { + res.send("Success") + next(); +}); + +app.get('/bar', (req, res, next) => { + throw new Error('bar'); +}); + +app.get('/baz', (req, res, next) => { + try { + throw new Error('hey there') + } + catch(e) { + Sentry.captureException(e); + } + res.send("Caught Exception: Baz") + next(); +}); + +app.use(Sentry.Handlers.errorHandler()); + +const server = app.listen(0, () => { + const port = server.address().port; + http.get(`http://localhost:${port}/foo`); + http.get(`http://localhost:${port}/bar`); + http.get(`http://localhost:${port}/bar`); + http.get(`http://localhost:${port}/baz`); +}); diff --git a/packages/node/test/manual/release-health/single-session/caught-exception-errored-session.js b/packages/node/test/manual/release-health/single-session/caught-exception-errored-session.js index a8d8a8eb6834..0840e031983b 100644 --- a/packages/node/test/manual/release-health/single-session/caught-exception-errored-session.js +++ b/packages/node/test/manual/release-health/single-session/caught-exception-errored-session.js @@ -1,5 +1,5 @@ const Sentry = require('../../../../dist'); -const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('./test-utils'); +const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); let sessionCounter = 0; process.on('exit', ()=> { diff --git a/packages/node/test/manual/release-health/single-session/healthy-session.js b/packages/node/test/manual/release-health/single-session/healthy-session.js index 8bbb6c6ac3df..bb9a9518688d 100644 --- a/packages/node/test/manual/release-health/single-session/healthy-session.js +++ b/packages/node/test/manual/release-health/single-session/healthy-session.js @@ -1,5 +1,5 @@ const Sentry = require('../../../../dist'); -const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('./test-utils'); +const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); let sessionCounter = 0; process.on('exit', ()=> { diff --git a/packages/node/test/manual/release-health/single-session/uncaught-exception-crashed-session.js b/packages/node/test/manual/release-health/single-session/uncaught-exception-crashed-session.js index 455da78fa4da..9fefa64b6877 100644 --- a/packages/node/test/manual/release-health/single-session/uncaught-exception-crashed-session.js +++ b/packages/node/test/manual/release-health/single-session/uncaught-exception-crashed-session.js @@ -1,5 +1,5 @@ const Sentry = require('../../../../dist'); -const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('./test-utils'); +const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); process.on('exit', ()=> { if (process.exitCode !== 1) { diff --git a/packages/node/test/manual/release-health/single-session/unhandled-rejection-crashed-session.js b/packages/node/test/manual/release-health/single-session/unhandled-rejection-crashed-session.js index 3a472f5e1827..d03b86f32956 100644 --- a/packages/node/test/manual/release-health/single-session/unhandled-rejection-crashed-session.js +++ b/packages/node/test/manual/release-health/single-session/unhandled-rejection-crashed-session.js @@ -1,5 +1,5 @@ const Sentry = require('../../../../dist'); -const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('./test-utils'); +const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); let sessionCounter = 0; process.on('exit', () => { diff --git a/packages/node/test/manual/release-health/single-session/test-utils.js b/packages/node/test/manual/release-health/test-utils.js similarity index 100% rename from packages/node/test/manual/release-health/single-session/test-utils.js rename to packages/node/test/manual/release-health/test-utils.js diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts index 5ffd077c9790..ed449802c322 100644 --- a/packages/node/test/transports/http.test.ts +++ b/packages/node/test/transports/http.test.ts @@ -1,5 +1,5 @@ import { Session } from '@sentry/hub'; -import { Event, SessionStatus, Status, TransportOptions } from '@sentry/types'; +import { Event, SessionAggregates, SessionStatus, Status, TransportOptions } from '@sentry/types'; import { SentryError } from '@sentry/utils'; import * as http from 'http'; import * as HttpsProxyAgent from 'https-proxy-agent'; @@ -31,6 +31,10 @@ const sessionPayload: Session = { close: jest.fn(), toJSON: jest.fn(), }; +const sessionsPayload: SessionAggregates = { + attrs: { environment: 'test', release: '1.0' }, + aggregates: [{ started: '2021-03-17T16:00:00.000Z', exited: 1 }], +}; let mockReturnCode = 200; let mockHeaders = {}; @@ -115,6 +119,28 @@ describe('HTTPTransport', () => { } }); + test('send 200 request mode sessions', async () => { + const transport = createTransport({ dsn }); + await transport.sendSessionAggregates(sessionsPayload); + + const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; + assertBasicOptions(requestOptions, true); + expect(mockSetEncoding).toHaveBeenCalled(); + }); + + test('send 400 request mode session', async () => { + mockReturnCode = 400; + const transport = createTransport({ dsn }); + + try { + await transport.sendSessionAggregates(sessionsPayload); + } catch (e) { + const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; + assertBasicOptions(requestOptions, true); + expect(e).toEqual(new SentryError(`HTTP Error (${mockReturnCode})`)); + } + }); + test('send x-sentry-error header', async () => { mockReturnCode = 429; mockHeaders = { diff --git a/packages/node/test/transports/https.test.ts b/packages/node/test/transports/https.test.ts index eb3bd8679ab2..79c378c6cf4d 100644 --- a/packages/node/test/transports/https.test.ts +++ b/packages/node/test/transports/https.test.ts @@ -1,5 +1,5 @@ import { Session } from '@sentry/hub'; -import { TransportOptions } from '@sentry/types'; +import { SessionAggregates, TransportOptions } from '@sentry/types'; import { SentryError } from '@sentry/utils'; import * as https from 'https'; import * as HttpsProxyAgent from 'https-proxy-agent'; @@ -10,6 +10,10 @@ const mockSetEncoding = jest.fn(); const dsn = 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622'; const storePath = '/mysubpath/api/50622/store/'; const envelopePath = '/mysubpath/api/50622/envelope/'; +const sessionsPayload: SessionAggregates = { + attrs: { environment: 'test', release: '1.0' }, + aggregates: [{ started: '2021-03-17T16:00:00.000Z', exited: 1 }], +}; let mockReturnCode = 200; let mockHeaders = {}; @@ -100,6 +104,28 @@ describe('HTTPSTransport', () => { } }); + test('send 200 request mode session', async () => { + const transport = createTransport({ dsn }); + await transport.sendSessionAggregates(sessionsPayload); + + const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; + assertBasicOptions(requestOptions, true); + expect(mockSetEncoding).toHaveBeenCalled(); + }); + + test('send 400 request mode session', async () => { + mockReturnCode = 400; + const transport = createTransport({ dsn }); + + try { + await transport.sendSessionAggregates(sessionsPayload); + } catch (e) { + const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; + assertBasicOptions(requestOptions, true); + expect(e).toEqual(new SentryError(`HTTP Error (${mockReturnCode})`)); + } + }); + test('send x-sentry-error header', async () => { mockReturnCode = 429; mockHeaders = { diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index fc75c17c6d08..d6070384223d 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -21,7 +21,15 @@ export { Runtime } from './runtime'; export { CaptureContext, Scope, ScopeContext } from './scope'; export { SdkInfo } from './sdkinfo'; export { SdkMetadata } from './sdkmetadata'; -export { Session, SessionContext, SessionStatus } from './session'; +export { + SessionAggregates, + AggregationCounts, + Session, + SessionContext, + SessionStatus, + RequestSessionStatus, + SessionFlusher, +} from './session'; export { Severity } from './severity'; export { Span, SpanContext } from './span'; export { StackFrame } from './stackframe'; diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts index 77c6204e499d..ee853f279d0d 100644 --- a/packages/types/src/scope.ts +++ b/packages/types/src/scope.ts @@ -3,7 +3,7 @@ import { Context, Contexts } from './context'; import { EventProcessor } from './eventprocessor'; import { Extra, Extras } from './extra'; import { Primitive } from './misc'; -import { Session } from './session'; +import { RequestSessionStatus, Session } from './session'; import { Severity } from './severity'; import { Span } from './span'; import { Transaction } from './transaction'; @@ -20,6 +20,7 @@ export interface ScopeContext { contexts: Contexts; tags: { [key: string]: Primitive }; fingerprint: string[]; + requestSession: { status: RequestSessionStatus }; } /** diff --git a/packages/types/src/session.ts b/packages/types/src/session.ts index c31a58224155..87c5308ae7aa 100644 --- a/packages/types/src/session.ts +++ b/packages/types/src/session.ts @@ -61,3 +61,47 @@ export enum SessionStatus { /** JSDoc */ Abnormal = 'abnormal', } + +export enum RequestSessionStatus { + /** JSDoc */ + Ok = 'ok', + /** JSDoc */ + Errored = 'errored', + /** JSDoc */ + Crashed = 'crashed', +} + +/** JSDoc */ +export interface SessionAggregates { + attrs?: { + environment?: string; + release?: string; + }; + aggregates: Array; +} + +export interface SessionFlusher { + readonly flushTimeout: number; + + /** + * Increments the Session Status bucket in SessionAggregates Object corresponding to the status of the session + * captured + */ + incrementSessionStatusCount(): void; + + /** Submits the aggregates request mode sessions to Sentry */ + sendSessionAggregates(sessionAggregates: SessionAggregates): void; + + /** Empties Aggregate Buckets and Sends them to Transport Buffer */ + flush(): void; + + /** Clears setInterval and calls flush */ + close(): void; +} + +export interface AggregationCounts { + started: string; + errored?: number; + exited?: number; + crashed?: number; +} diff --git a/packages/types/src/transport.ts b/packages/types/src/transport.ts index 1447886fc7ae..b98dd2e3202f 100644 --- a/packages/types/src/transport.ts +++ b/packages/types/src/transport.ts @@ -2,7 +2,7 @@ import { DsnLike } from './dsn'; import { Event } from './event'; import { Response } from './response'; import { SdkMetadata } from './sdkmetadata'; -import { Session } from './session'; +import { Session, SessionAggregates } from './session'; /** Transport used sending data to Sentry */ export interface Transport { @@ -20,6 +20,13 @@ export interface Transport { */ sendSession?(session: Session): PromiseLike; + /** + * Sends the request mode session aggregates to the Envelope endpoint in Sentry. + * + * @param sessionAggregates Session Aggregates that should be sent to Sentry. + */ + sendSessionAggregates?(sessionAggregates: SessionAggregates): PromiseLike; + /** * Call this function to wait until all pending requests have been sent. * From 8558c124f0179a2e185fe73e56505db52c3d791c Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 09:54:12 +0200 Subject: [PATCH 02/10] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kamil Ogórek --- packages/hub/src/session.ts | 10 +++++----- packages/node/src/handlers.ts | 13 ++++++------- packages/types/src/session.ts | 1 - 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/packages/hub/src/session.ts b/packages/hub/src/session.ts index c155d66a9fa4..a9ad3544abec 100644 --- a/packages/hub/src/session.ts +++ b/packages/hub/src/session.ts @@ -145,7 +145,7 @@ type releaseHealthAttributes = { */ export class SessionFlusher implements SessionFlusherInterface { public readonly flushTimeout: number = 60; - private _pendingAggregates: { [key: number]: AggregationCounts } = {}; + private _pendingAggregates: Record = {}; private _sessionAttrs: releaseHealthAttributes; private _intervalId: ReturnType; private _isEnabled: boolean = true; @@ -187,7 +187,7 @@ export class SessionFlusher implements SessionFlusherInterface { const sessionAggregates: SessionAggregates = { attrs: this._sessionAttrs, - aggregates: aggregates, + aggregates, }; return dropUndefinedKeys(sessionAggregates); } @@ -240,13 +240,13 @@ export class SessionFlusher implements SessionFlusherInterface { switch (status) { case RequestSessionStatus.Errored: - aggregationCounts.errored = aggregationCounts.errored !== undefined ? aggregationCounts.errored + 1 : 1; + aggregationCounts.errored = (aggregationCounts.errored || 0) + 1; return aggregationCounts.errored; case RequestSessionStatus.Ok: - aggregationCounts.exited = aggregationCounts.exited !== undefined ? aggregationCounts.exited + 1 : 1; + aggregationCounts.exited = (aggregationCounts.exited || 0) + 1; return aggregationCounts.exited; case RequestSessionStatus.Crashed: - aggregationCounts.crashed = aggregationCounts.crashed !== undefined ? aggregationCounts.crashed + 1 : 1; + aggregationCounts.crashed = (aggregationCounts.crashed || 0) + 1; return aggregationCounts.crashed; } } diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index bff33a252ae3..1fb7435e9b43 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -426,14 +426,13 @@ export function requestHandler( res.once('finish', () => { const client = currentHub.getClient(); - if (isAutoSessionTrackingEnabled(client)) { + + if (!client || !isAutoSessionTrackingEnabled(client)) { setImmediate(() => { - if (client) { - // Calling _captureRequestSession to capture request session at the end of the request by incrementing - // the correct SessionAggregates bucket i.e. crashed, errored or exited - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - (client as any)._captureRequestSession(); - } + // Calling _captureRequestSession to capture request session at the end of the request by incrementing + // the correct SessionAggregates bucket i.e. crashed, errored or exited + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + (client as any)._captureRequestSession(); }); } }); diff --git a/packages/types/src/session.ts b/packages/types/src/session.ts index 87c5308ae7aa..fd54f10b5aec 100644 --- a/packages/types/src/session.ts +++ b/packages/types/src/session.ts @@ -81,7 +81,6 @@ export interface SessionAggregates { } export interface SessionFlusher { - readonly flushTimeout: number; /** * Increments the Session Status bucket in SessionAggregates Object corresponding to the status of the session From 158d93d7b4e344ffa9ba2c89b608b45411390df8 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 10:24:37 +0200 Subject: [PATCH 03/10] ref: Only have sendSession --- packages/browser/src/transports/base.ts | 1 + packages/core/src/index.ts | 2 +- packages/core/src/request.ts | 29 ++++----------- packages/core/test/lib/request.test.ts | 6 ++-- packages/hub/src/session.ts | 6 ++-- packages/hub/test/sessionflusher.test.ts | 18 +++++----- packages/node/src/handlers.ts | 13 +++---- packages/node/src/transports/base.ts | 7 +++- packages/node/src/transports/http.ts | 11 ++---- packages/node/src/transports/https.ts | 11 ++---- .../aggregates-disable-single-session.js | 36 +++++++++---------- packages/node/test/transports/http.test.ts | 4 +-- packages/node/test/transports/https.test.ts | 4 +-- packages/types/src/request.ts | 2 +- packages/types/src/transport.ts | 11 ++---- 15 files changed, 64 insertions(+), 97 deletions(-) diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index de093c4f33e9..7318d46cdfcc 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -15,6 +15,7 @@ const CATEGORY_MAPPING: { event: 'error', transaction: 'transaction', session: 'session', + sessions: 'session', }; /** Base Transport class implementation */ diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 53848b1df5a5..5fe8450de3ed 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -17,7 +17,7 @@ export { addGlobalEventProcessor, getCurrentHub, getHubFromCarrier, Hub, makeMai export { API } from './api'; export { BaseClient } from './baseclient'; export { BackendClass, BaseBackend } from './basebackend'; -export { eventToSentryRequest, sessionToSentryRequest, sessionAggregatesToSentryRequest } from './request'; +export { eventToSentryRequest, sessionToSentryRequest } from './request'; export { initAndBind, ClientClass } from './sdk'; export { NoopTransport } from './transports/noop'; export { SDK_VERSION } from './version'; diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts index 3ee62d64fd4c..212c73b72ab2 100644 --- a/packages/core/src/request.ts +++ b/packages/core/src/request.ts @@ -28,38 +28,23 @@ function enhanceEventWithSdkInfo(event: Event, sdkInfo?: SdkInfo): Event { } /** Creates a SentryRequest from a Session. */ -export function sessionToSentryRequest(session: Session, api: API): SentryRequest { +export function sessionToSentryRequest(session: Session | SessionAggregates, api: API): SentryRequest { const sdkInfo = getSdkMetadataForEnvelopeHeader(api); const envelopeHeaders = JSON.stringify({ sent_at: new Date().toISOString(), ...(sdkInfo && { sdk: sdkInfo }), }); + let type: SentryRequestType = 'session'; + if ('aggregates' in session) { + type = 'sessions'; + } const itemHeaders = JSON.stringify({ - type: 'session', + type, }); return { body: `${envelopeHeaders}\n${itemHeaders}\n${JSON.stringify(session)}`, - type: 'session', - url: api.getEnvelopeEndpointWithUrlEncodedAuth(), - }; -} - -/** Creates a SentryRequest from an Aggregates of Request mode sessions */ -export function sessionAggregatesToSentryRequest(sessionAggregates: SessionAggregates, api: API): SentryRequest { - const sdkInfo = getSdkMetadataForEnvelopeHeader(api); - const envelopeHeaders = JSON.stringify({ - sent_at: new Date().toISOString(), - ...(sdkInfo && { sdk: sdkInfo }), - }); - // The server expects type `sessions` in headers for Session Aggregates payload - const itemHeaders = JSON.stringify({ - type: 'sessions', - }); - - return { - body: `${envelopeHeaders}\n${itemHeaders}\n${JSON.stringify(sessionAggregates)}`, - type: 'sessions' as SentryRequestType, + type, url: api.getEnvelopeEndpointWithUrlEncodedAuth(), }; } diff --git a/packages/core/test/lib/request.test.ts b/packages/core/test/lib/request.test.ts index 076282bba998..589cf40769c5 100644 --- a/packages/core/test/lib/request.test.ts +++ b/packages/core/test/lib/request.test.ts @@ -1,7 +1,7 @@ import { DebugMeta, Event, SentryRequest, TransactionSamplingMethod } from '@sentry/types'; import { API } from '../../src/api'; -import { eventToSentryRequest, sessionAggregatesToSentryRequest } from '../../src/request'; +import { eventToSentryRequest, sessionToSentryRequest } from '../../src/request'; const api = new API('https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', { sdk: { @@ -126,13 +126,13 @@ describe('eventToSentryRequest', () => { }); }); -describe('sessionAggregatesToSentryRequest', () => { +describe('sessionToSentryRequest', () => { it('test envelope creation for aggregateSessions', () => { const aggregatedSession = { attrs: { release: '1.0.x', environment: 'prod' }, aggregates: [{ started: '2021-04-08T12:18:00.000Z', exited: 2 }], }; - const result = sessionAggregatesToSentryRequest(aggregatedSession, api); + const result = sessionToSentryRequest(aggregatedSession, api); const [envelopeHeaderString, itemHeaderString, sessionString] = result.body.split('\n'); diff --git a/packages/hub/src/session.ts b/packages/hub/src/session.ts index a9ad3544abec..e91f4f3900e7 100644 --- a/packages/hub/src/session.ts +++ b/packages/hub/src/session.ts @@ -160,11 +160,11 @@ export class SessionFlusher implements SessionFlusherInterface { /** Sends session aggregates to Transport */ public sendSessionAggregates(sessionAggregates: SessionAggregates): void { - if (!this._transport.sendSessionAggregates) { - logger.warn("Dropping session because custom transport doesn't implement sendSessionAggregates"); + if (!this._transport.sendSession) { + logger.warn("Dropping session because custom transport doesn't implement sendSession"); return; } - this._transport.sendSessionAggregates(sessionAggregates).then(null, reason => { + this._transport.sendSession(sessionAggregates).then(null, reason => { logger.error(`Error while sending session: ${reason}`); }); } diff --git a/packages/hub/test/sessionflusher.test.ts b/packages/hub/test/sessionflusher.test.ts index 7f3ae2cd776b..486ea9c9ca97 100644 --- a/packages/hub/test/sessionflusher.test.ts +++ b/packages/hub/test/sessionflusher.test.ts @@ -3,19 +3,19 @@ import { RequestSessionStatus, Status } from '@sentry/types'; import { SessionFlusher } from '../src'; describe('Session Flusher', () => { - let sendSessionAggregates: jest.Mock; + let sendSession: jest.Mock; let transport: { sendEvent: jest.Mock; - sendSessionAggregates: jest.Mock; + sendSession: jest.Mock; close: jest.Mock; }; beforeEach(() => { jest.useFakeTimers(); - sendSessionAggregates = jest.fn(() => Promise.resolve({ status: Status.Success })); + sendSession = jest.fn(() => Promise.resolve({ status: Status.Success })); transport = { sendEvent: jest.fn(), - sendSessionAggregates, + sendSession, close: jest.fn(), }; }); @@ -80,12 +80,12 @@ describe('Session Flusher', () => { (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); (flusher as any)._incrementSessionStatusCount(RequestSessionStatus.Ok, date); - expect(sendSessionAggregates).toHaveBeenCalledTimes(0); + expect(sendSession).toHaveBeenCalledTimes(0); jest.advanceTimersByTime(61000); expect(flusherFlushFunc).toHaveBeenCalledTimes(1); - expect(sendSessionAggregates).toHaveBeenCalledWith( + expect(sendSession).toHaveBeenCalledWith( expect.objectContaining({ attrs: { release: '1.0.0', environment: 'dev' }, aggregates: [{ started: '2021-04-08T12:18:00.000Z', exited: 2 }], @@ -97,10 +97,10 @@ describe('Session Flusher', () => { const flusher = new SessionFlusher(transport, { release: '1.0.0', environment: 'dev' }); const flusherFlushFunc = jest.spyOn(flusher, 'flush'); - expect(sendSessionAggregates).toHaveBeenCalledTimes(0); + expect(sendSession).toHaveBeenCalledTimes(0); jest.advanceTimersByTime(61000); expect(flusherFlushFunc).toHaveBeenCalledTimes(1); - expect(sendSessionAggregates).toHaveBeenCalledTimes(0); + expect(sendSession).toHaveBeenCalledTimes(0); }); test('calling close on SessionFlusher should disable SessionFlusher', () => { @@ -118,7 +118,7 @@ describe('Session Flusher', () => { flusher.close(); expect(flusherFlushFunc).toHaveBeenCalledTimes(1); - expect(sendSessionAggregates).toHaveBeenCalledWith( + expect(sendSession).toHaveBeenCalledWith( expect.objectContaining({ attrs: { release: '1.0.x' }, aggregates: [{ started: '2021-04-08T12:18:00.000Z', exited: 2 }], diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 1fb7435e9b43..bff33a252ae3 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -426,13 +426,14 @@ export function requestHandler( res.once('finish', () => { const client = currentHub.getClient(); - - if (!client || !isAutoSessionTrackingEnabled(client)) { + if (isAutoSessionTrackingEnabled(client)) { setImmediate(() => { - // Calling _captureRequestSession to capture request session at the end of the request by incrementing - // the correct SessionAggregates bucket i.e. crashed, errored or exited - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - (client as any)._captureRequestSession(); + if (client) { + // Calling _captureRequestSession to capture request session at the end of the request by incrementing + // the correct SessionAggregates bucket i.e. crashed, errored or exited + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + (client as any)._captureRequestSession(); + } }); } }); diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts index 688d0511e6cb..d4f3b79b4e6d 100644 --- a/packages/node/src/transports/base.ts +++ b/packages/node/src/transports/base.ts @@ -6,6 +6,7 @@ import { SentryRequest, SentryRequestType, Session, + SessionAggregates, Status, Transport, TransportOptions, @@ -50,6 +51,7 @@ const CATEGORY_MAPPING: { event: 'error', transaction: 'transaction', session: 'session', + sessions: 'session', }; /** Base Transport class implementation */ @@ -198,7 +200,10 @@ export abstract class BaseTransport implements Transport { } /** JSDoc */ - protected async _send(sentryReq: SentryRequest, originalPayload?: Event | Session): Promise { + protected async _send( + sentryReq: SentryRequest, + originalPayload?: Event | Session | SessionAggregates, + ): Promise { if (!this.module) { throw new SentryError('No module available'); } diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts index 5a4dff8a9bd5..a9234d830876 100644 --- a/packages/node/src/transports/http.ts +++ b/packages/node/src/transports/http.ts @@ -1,4 +1,4 @@ -import { eventToSentryRequest, sessionAggregatesToSentryRequest, sessionToSentryRequest } from '@sentry/core'; +import { eventToSentryRequest, sessionToSentryRequest } from '@sentry/core'; import { Event, Response, Session, SessionAggregates, TransportOptions } from '@sentry/types'; import * as http from 'http'; @@ -26,14 +26,7 @@ export class HTTPTransport extends BaseTransport { /** * @inheritDoc */ - public sendSession(session: Session): PromiseLike { + public sendSession(session: Session | SessionAggregates): PromiseLike { return this._send(sessionToSentryRequest(session, this._api), session); } - - /** - * @inheritDoc - */ - public sendSessionAggregates(sessionAggregates: SessionAggregates): PromiseLike { - return this._send(sessionAggregatesToSentryRequest(sessionAggregates, this._api)); - } } diff --git a/packages/node/src/transports/https.ts b/packages/node/src/transports/https.ts index a23352abc891..d6c312608504 100644 --- a/packages/node/src/transports/https.ts +++ b/packages/node/src/transports/https.ts @@ -1,4 +1,4 @@ -import { eventToSentryRequest, sessionAggregatesToSentryRequest, sessionToSentryRequest } from '@sentry/core'; +import { eventToSentryRequest, sessionToSentryRequest } from '@sentry/core'; import { Event, Response, Session, SessionAggregates, TransportOptions } from '@sentry/types'; import * as https from 'https'; @@ -26,14 +26,7 @@ export class HTTPSTransport extends BaseTransport { /** * @inheritDoc */ - public sendSession(session: Session): PromiseLike { + public sendSession(session: Session | SessionAggregates): PromiseLike { return this._send(sessionToSentryRequest(session, this._api), session); } - - /** - * @inheritDoc - */ - public sendSessionAggregates(sessionAggregates: SessionAggregates): PromiseLike { - return this._send(sessionAggregatesToSentryRequest(sessionAggregates, this._api)); - } } diff --git a/packages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js b/packages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js index 6d93f43f74ea..c873d6e11faa 100644 --- a/packages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js +++ b/packages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js @@ -7,35 +7,33 @@ const { assertSessions, BaseDummyTransport } = require('../test-utils'); function cleanUpAndExitSuccessfully() { console.log('SUCCESS: Session Aggregates payload is correct!'); server.close(); - clearInterval(flusher._intervalId) + clearInterval(flusher._intervalId); process.exit(0); } function assertSessionAggregates(session, expected) { // For loop is added here just in the rare occasion that the session count do not land in the same aggregate // bucket - session.aggregates.forEach(function (_, idx) { + session.aggregates.forEach(function(_, idx) { delete session.aggregates[idx].started; // Session Aggregates keys need to be ordered for JSON.stringify comparison - const ordered = Object.keys(session.aggregates[idx]).sort().reduce( - (obj, key) => { + const ordered = Object.keys(session.aggregates[idx]) + .sort() + .reduce((obj, key) => { obj[key] = session.aggregates[idx][key]; return obj; - }, - {} - ); + }, {}); session.aggregates[idx] = ordered; - }) - assertSessions(session, expected) + }); + assertSessions(session, expected); } class DummyTransport extends BaseDummyTransport { sendSession(session) { - console.error('FAIL: Received Single Session which should have been deleted in favor of Session Aggregates!'); - process.exit(1); - } - sendSessionAggregates(session) { - assertSessionAggregates(session, {"attrs":{"release":"1.1"},"aggregates":[{"crashed":2,"errored":1,"exited":1}]}) + assertSessionAggregates(session, { + attrs: { release: '1.1' }, + aggregates: [{ crashed: 2, errored: 1, exited: 1 }], + }); cleanUpAndExitSuccessfully(); @@ -63,9 +61,8 @@ const flusher = Sentry.getCurrentHub().getClient()._sessionFlusher; clearInterval(flusher._intervalId); flusher._intervalId = setInterval(() => flusher.flush(), 1000); - app.get('/foo', (req, res, next) => { - res.send("Success") + res.send('Success'); next(); }); @@ -75,12 +72,11 @@ app.get('/bar', (req, res, next) => { app.get('/baz', (req, res, next) => { try { - throw new Error('hey there') - } - catch(e) { + throw new Error('hey there'); + } catch (e) { Sentry.captureException(e); } - res.send("Caught Exception: Baz") + res.send('Caught Exception: Baz'); next(); }); diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts index ed449802c322..14ea735eb19e 100644 --- a/packages/node/test/transports/http.test.ts +++ b/packages/node/test/transports/http.test.ts @@ -121,7 +121,7 @@ describe('HTTPTransport', () => { test('send 200 request mode sessions', async () => { const transport = createTransport({ dsn }); - await transport.sendSessionAggregates(sessionsPayload); + await transport.sendSession(sessionsPayload); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions, true); @@ -133,7 +133,7 @@ describe('HTTPTransport', () => { const transport = createTransport({ dsn }); try { - await transport.sendSessionAggregates(sessionsPayload); + await transport.sendSession(sessionsPayload); } catch (e) { const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions, true); diff --git a/packages/node/test/transports/https.test.ts b/packages/node/test/transports/https.test.ts index 79c378c6cf4d..0991e9332848 100644 --- a/packages/node/test/transports/https.test.ts +++ b/packages/node/test/transports/https.test.ts @@ -106,7 +106,7 @@ describe('HTTPSTransport', () => { test('send 200 request mode session', async () => { const transport = createTransport({ dsn }); - await transport.sendSessionAggregates(sessionsPayload); + await transport.sendSession(sessionsPayload); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions, true); @@ -118,7 +118,7 @@ describe('HTTPSTransport', () => { const transport = createTransport({ dsn }); try { - await transport.sendSessionAggregates(sessionsPayload); + await transport.sendSession(sessionsPayload); } catch (e) { const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions, true); diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index 6087c7317b2c..99d2b391bd92 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -1,5 +1,5 @@ /** Possible SentryRequest types that can be used to make a distinction between Sentry features */ -export type SentryRequestType = 'event' | 'transaction' | 'session'; +export type SentryRequestType = 'event' | 'transaction' | 'session' | 'sessions'; /** A generic client request. */ export interface SentryRequest { diff --git a/packages/types/src/transport.ts b/packages/types/src/transport.ts index b98dd2e3202f..d6c20bbdf507 100644 --- a/packages/types/src/transport.ts +++ b/packages/types/src/transport.ts @@ -16,16 +16,9 @@ export interface Transport { /** * Sends the session to the Envelope endpoint in Sentry. * - * @param session Session that should be sent to Sentry. + * @param session Session that should be sent to Sentry | Session Aggregates that should be sent to Sentry. */ - sendSession?(session: Session): PromiseLike; - - /** - * Sends the request mode session aggregates to the Envelope endpoint in Sentry. - * - * @param sessionAggregates Session Aggregates that should be sent to Sentry. - */ - sendSessionAggregates?(sessionAggregates: SessionAggregates): PromiseLike; + sendSession?(session: Session | SessionAggregates): PromiseLike; /** * Call this function to wait until all pending requests have been sent. From 5a9058e3c34059c826b08743cbd97838e89a9832 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 10:42:27 +0200 Subject: [PATCH 04/10] ref: RequestSession getter/setter on scope --- packages/hub/src/scope.ts | 19 +++++++++++++++++-- packages/hub/src/session.ts | 25 ++++++++++++------------- packages/node/src/client.ts | 12 +++--------- packages/node/src/handlers.ts | 10 ++++------ packages/types/src/index.ts | 3 ++- packages/types/src/scope.ts | 18 ++++++++++++++---- packages/types/src/session.ts | 7 +++++-- 7 files changed, 57 insertions(+), 37 deletions(-) diff --git a/packages/hub/src/scope.ts b/packages/hub/src/scope.ts index a0c9e6e0dd70..bb84c77a7281 100644 --- a/packages/hub/src/scope.ts +++ b/packages/hub/src/scope.ts @@ -10,7 +10,7 @@ import { Extra, Extras, Primitive, - RequestSessionStatus, + RequestSession, Scope as ScopeInterface, ScopeContext, Severity, @@ -67,7 +67,7 @@ export class Scope implements ScopeInterface { protected _session?: Session; /** Request Mode Session Status */ - protected _requestSession?: { status?: RequestSessionStatus }; + protected _requestSession?: RequestSession; /** * Inherit values from the parent scope. @@ -127,6 +127,21 @@ export class Scope implements ScopeInterface { return this._user; } + /** + * @inheritDoc + */ + public getRequestSession(): RequestSession | undefined { + return this._requestSession; + } + + /** + * @inheritDoc + */ + public setRequestSession(requestSession?: RequestSession): this { + this._requestSession = requestSession; + return this; + } + /** * @inheritDoc */ diff --git a/packages/hub/src/session.ts b/packages/hub/src/session.ts index e91f4f3900e7..9a6b768ed62b 100644 --- a/packages/hub/src/session.ts +++ b/packages/hub/src/session.ts @@ -4,7 +4,7 @@ import { Session as SessionInterface, SessionAggregates, SessionContext, - SessionFlusher as SessionFlusherInterface, + SessionFlusherLike, SessionStatus, Transport, } from '@sentry/types'; @@ -135,7 +135,7 @@ export class Session implements SessionInterface { } } -type releaseHealthAttributes = { +type ReleaseHealthAttributes = { environment?: string; release: string; }; @@ -143,15 +143,15 @@ type releaseHealthAttributes = { /** * @inheritdoc */ -export class SessionFlusher implements SessionFlusherInterface { +export class SessionFlusher implements SessionFlusherLike { public readonly flushTimeout: number = 60; private _pendingAggregates: Record = {}; - private _sessionAttrs: releaseHealthAttributes; + private _sessionAttrs: ReleaseHealthAttributes; private _intervalId: ReturnType; private _isEnabled: boolean = true; private _transport: Transport; - constructor(transport: Transport, attrs: releaseHealthAttributes) { + constructor(transport: Transport, attrs: ReleaseHealthAttributes) { this._transport = transport; // Call to setInterval, so that flush is called every 60 seconds this._intervalId = setInterval(() => this.flush(), this.flushTimeout * 1000); @@ -170,7 +170,7 @@ export class SessionFlusher implements SessionFlusherInterface { } /** Checks if `pendingAggregates` has entries, and if it does flushes them by calling `sendSessions` */ - flush(): void { + public flush(): void { const sessionAggregates = this.getSessionAggregates(); if (sessionAggregates.aggregates.length === 0) { return; @@ -180,7 +180,7 @@ export class SessionFlusher implements SessionFlusherInterface { } /** Massages the entries in `pendingAggregates` and returns aggregated sessions */ - getSessionAggregates(): SessionAggregates { + public getSessionAggregates(): SessionAggregates { const aggregates: AggregationCounts[] = Object.keys(this._pendingAggregates).map((key: string) => { return this._pendingAggregates[parseInt(key)]; }); @@ -193,7 +193,7 @@ export class SessionFlusher implements SessionFlusherInterface { } /** JSDoc */ - close(): void { + public close(): void { clearInterval(this._intervalId); this._isEnabled = false; this.flush(); @@ -201,7 +201,7 @@ export class SessionFlusher implements SessionFlusherInterface { /** * Wrapper function for _incrementSessionStatusCount that checks if the instance of SessionFlusher is enabled then - * fetches the session status of the request from `_requestSession.status` on the scope and passes them to + * fetches the session status of the request from `Scope.getRequestSession().status` on the scope and passes them to * `_incrementSessionStatusCount` along with the start date */ public incrementSessionStatusCount(): void { @@ -209,14 +209,13 @@ export class SessionFlusher implements SessionFlusherInterface { return; } const scope = getCurrentHub().getScope(); - /* eslint-disable @typescript-eslint/no-unsafe-member-access */ - const requestSession = (scope as any)._requestSession; + const requestSession = scope?.getRequestSession(); if (requestSession && requestSession.status) { this._incrementSessionStatusCount(requestSession.status, new Date()); // This is not entirely necessarily but is added as a safe guard to indicate the bounds of a request and so in // case captureRequestSession is called more than once to prevent double count - (scope as any)._requestSession = undefined; + scope?.setRequestSession(undefined); /* eslint-enable @typescript-eslint/no-unsafe-member-access */ } @@ -228,7 +227,7 @@ export class SessionFlusher implements SessionFlusherInterface { */ private _incrementSessionStatusCount(status: RequestSessionStatus, date: Date): number { // Truncate minutes and seconds on Session Started attribute to have one minute bucket keys - const sessionStartedTrunc: number = new Date(date).setSeconds(0, 0); + const sessionStartedTrunc = new Date(date).setSeconds(0, 0); this._pendingAggregates[sessionStartedTrunc] = this._pendingAggregates[sessionStartedTrunc] || {}; // corresponds to aggregated sessions in one specific minute bucket diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index c68278cbe684..11a95e37a70a 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -38,13 +38,11 @@ export class NodeClient extends BaseClient { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined { - /* eslint-disable @typescript-eslint/no-unsafe-member-access */ - // Check if the flag `autoSessionTracking` is enabled, and if `_sessionFlusher` exists because it is initialised only // when the `requestHandler` middleware is used, and hence the expectation is to have SessionAggregates payload // sent to the Server only when the `requestHandler` middleware is used if (this._options.autoSessionTracking && this._sessionFlusher && scope) { - const requestSession = (scope as any)._requestSession; + const requestSession = scope.getRequestSession(); // Necessary checks to ensure this is code block is executed only within a request // Should override the status only if `requestSession.status` is `Ok`, which is its initial stage @@ -52,7 +50,6 @@ export class NodeClient extends BaseClient { requestSession.status = RequestSessionStatus.Errored; } } - /* eslint-enable @typescript-eslint/no-unsafe-member-access */ return super.captureException(exception, hint, scope); } @@ -71,16 +68,13 @@ export class NodeClient extends BaseClient { // If the event is of type Exception, then a request session should be captured if (isException) { - /* eslint-disable @typescript-eslint/no-unsafe-member-access */ - const requestSession = (scope as any)._requestSession; + const requestSession = scope.getRequestSession(); // Ensure that this is happening within the bounds of a request, and make sure not to override // Session Status if Errored / Crashed if (requestSession && requestSession.status === RequestSessionStatus.Ok) { - (scope as any)._requestSession.status = RequestSessionStatus.Errored; + requestSession.status = RequestSessionStatus.Errored; } - - /* eslint-enable @typescript-eslint/no-unsafe-member-access */ } } diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index bff33a252ae3..ca2d4aa73407 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -417,9 +417,8 @@ export function requestHandler( if (isAutoSessionTrackingEnabled(client)) { const scope = currentHub.getScope(); if (scope) { - // Set `status` of `_requestSession` to Ok, at the beginning of the request - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + // Set `status` of `RequestSession` to Ok, at the beginning of the request + scope.setRequestSession({ status: RequestSessionStatus.Ok }); } } }); @@ -500,14 +499,14 @@ export function errorHandler(options?: { const client = getCurrentHub().getClient(); if (client && isAutoSessionTrackingEnabled(client)) { - /* eslint-disable @typescript-eslint/no-unsafe-member-access */ // Check if the `SessionFlusher` is instantiated on the client to go into this branch that marks the // `requestSession.status` as `Crashed`, and this check is necessary because the `SessionFlusher` is only // instantiated when the the`requestHandler` middleware is initialised, which indicates that we should be // running in SessionAggregates mode + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access const isSessionAggregatesMode = (client as any)._sessionFlusher !== undefined; if (isSessionAggregatesMode) { - const requestSession = (_scope as any)._requestSession; + const requestSession = _scope.getRequestSession(); // If an error bubbles to the `errorHandler`, then this is an unhandled error, and should be reported as a // Crashed session. The `_requestSession.status` is checked to ensure that this error is happening within // the bounds of a request, and if so the status is updated @@ -515,7 +514,6 @@ export function errorHandler(options?: { requestSession.status = RequestSessionStatus.Crashed; } } - /* eslint-enable @typescript-eslint/no-unsafe-member-access */ const eventId = captureException(error); // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index d6070384223d..78f5c862a127 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -27,8 +27,9 @@ export { Session, SessionContext, SessionStatus, + RequestSession, RequestSessionStatus, - SessionFlusher, + SessionFlusherLike, } from './session'; export { Severity } from './severity'; export { Span, SpanContext } from './span'; diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts index ee853f279d0d..c3b8f6c5b99c 100644 --- a/packages/types/src/scope.ts +++ b/packages/types/src/scope.ts @@ -3,7 +3,7 @@ import { Context, Contexts } from './context'; import { EventProcessor } from './eventprocessor'; import { Extra, Extras } from './extra'; import { Primitive } from './misc'; -import { RequestSessionStatus, Session } from './session'; +import { RequestSession, Session } from './session'; import { Severity } from './severity'; import { Span } from './span'; import { Transaction } from './transaction'; @@ -20,7 +20,7 @@ export interface ScopeContext { contexts: Contexts; tags: { [key: string]: Primitive }; fingerprint: string[]; - requestSession: { status: RequestSessionStatus }; + requestSession: RequestSession; } /** @@ -112,15 +112,25 @@ export interface Scope { */ getTransaction(): Transaction | undefined; + /** + * Returns the `Session` if there is one + */ + getSession(): Session | undefined; + /** * Sets the `Session` on the scope */ setSession(session?: Session): this; /** - * Returns the `Session` if there is one + * Returns the `RequestSession` if there is one */ - getSession(): Session | undefined; + getRequestSession(): RequestSession | undefined; + + /** + * Sets the `RequestSession` on the scope + */ + setRequestSession(requestSession?: RequestSession): this; /** * Updates the scope with provided data. Can work in three variations: diff --git a/packages/types/src/session.ts b/packages/types/src/session.ts index fd54f10b5aec..2875a30d309c 100644 --- a/packages/types/src/session.ts +++ b/packages/types/src/session.ts @@ -29,6 +29,10 @@ export interface Session extends SessionContext { }; } +export interface RequestSession { + status?: RequestSessionStatus; +} + /** * Session Context */ @@ -80,8 +84,7 @@ export interface SessionAggregates { aggregates: Array; } -export interface SessionFlusher { - +export interface SessionFlusherLike { /** * Increments the Session Status bucket in SessionAggregates Object corresponding to the status of the session * captured From 42c6f9f7285155cc69a2cd61dc7509e50ff37723 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 10:51:51 +0200 Subject: [PATCH 05/10] fix: Tests getRequestSession --- packages/hub/test/scope.test.ts | 19 ++++++---- packages/node/test/client.test.ts | 58 ++++++++++++++--------------- packages/node/test/handlers.test.ts | 22 +++++------ 3 files changed, 51 insertions(+), 48 deletions(-) diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts index 9c405e8091e8..dfb32e6f4a80 100644 --- a/packages/hub/test/scope.test.ts +++ b/packages/hub/test/scope.test.ts @@ -131,9 +131,9 @@ describe('Scope', () => { test('_requestSession clone', () => { const parentScope = new Scope(); - (parentScope as any)._requestSession = { status: RequestSessionStatus.Errored }; + parentScope.setRequestSession({ status: RequestSessionStatus.Errored }); const scope = Scope.clone(parentScope); - expect((parentScope as any)._requestSession).toEqual((scope as any)._requestSession); + expect(parentScope.getRequestSession()).toEqual(scope.getRequestSession()); }); test('parent changed inheritance', () => { @@ -158,13 +158,16 @@ describe('Scope', () => { // Test that ensures if the status value of `status` of `_requestSession` is changed in a child scope // that it should also change in parent scope because we are copying the reference to the object const parentScope = new Scope(); - (parentScope as any)._requestSession = { status: RequestSessionStatus.Errored }; + parentScope.setRequestSession({ status: RequestSessionStatus.Errored }); const scope = Scope.clone(parentScope); - (scope as any)._requestSession.status = RequestSessionStatus.Ok; + const requestSession = scope.getRequestSession(); + if (requestSession) { + requestSession.status = RequestSessionStatus.Ok; + } - expect((parentScope as any)._requestSession).toEqual({ status: RequestSessionStatus.Ok }); - expect((scope as any)._requestSession).toEqual({ status: RequestSessionStatus.Ok }); + expect(parentScope.getRequestSession()).toEqual({ status: RequestSessionStatus.Ok }); + expect(scope.getRequestSession()).toEqual({ status: RequestSessionStatus.Ok }); }); }); @@ -356,7 +359,7 @@ describe('Scope', () => { scope.setUser({ id: '1' }); scope.setFingerprint(['abcd']); scope.addBreadcrumb({ message: 'test' }, 100); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); expect((scope as any)._extra).toEqual({ a: 2 }); scope.clear(); expect((scope as any)._extra).toEqual({}); @@ -383,7 +386,7 @@ describe('Scope', () => { scope.setUser({ id: '1337' }); scope.setLevel(Severity.Info); scope.setFingerprint(['foo']); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); }); test('given no data, returns the original scope', () => { diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts index 3ad1e5eeeef0..feece0fd7979 100644 --- a/packages/node/test/client.test.ts +++ b/packages/node/test/client.test.ts @@ -17,12 +17,12 @@ describe('NodeClient', () => { test('when autoSessionTracking is enabled, and requestHandler is not used -> requestStatus should not be set', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); client.captureException(new Error('test exception'), undefined, scope); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Ok); }); test('when autoSessionTracking is disabled -> requestStatus should not be set', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }); @@ -31,12 +31,12 @@ describe('NodeClient', () => { (client as any)._initSessionFlusher(); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); client.captureException(new Error('test exception'), undefined, scope); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Ok); }); test('when autoSessionTracking is enabled + requestSession status is Crashed -> requestStatus should not be overridden', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); @@ -45,12 +45,12 @@ describe('NodeClient', () => { (client as any)._initSessionFlusher(); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Crashed }; + scope.setRequestSession({ status: RequestSessionStatus.Crashed }); client.captureException(new Error('test exception'), undefined, scope); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Crashed); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Crashed); }); test('when autoSessionTracking is enabled + error occurs within request bounds -> requestStatus should be set to Errored', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); @@ -59,12 +59,12 @@ describe('NodeClient', () => { (client as any)._initSessionFlusher(); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); client.captureException(new Error('test exception'), undefined, scope); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Errored); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Errored); }); test('when autoSessionTracking is enabled + error occurs outside of request bounds -> requestStatus should not be set to Errored', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); @@ -76,7 +76,7 @@ describe('NodeClient', () => { client.captureException(new Error('test exception'), undefined, scope); - const requestSession = (scope as any)._requestSession; + const requestSession = scope.getRequestSession(); expect(requestSession).toEqual(undefined); }); }); @@ -89,15 +89,15 @@ describe('NodeClient', () => { (client as any)._initSessionFlusher(); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); client.captureEvent( { message: 'message', exception: { values: [{ type: 'exception type 1' }] } }, undefined, scope, ); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Ok); }); test('When captureEvent is called with an exception, requestSession status should be set to Errored', () => { @@ -107,12 +107,12 @@ describe('NodeClient', () => { (client as any)._initSessionFlusher(); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); client.captureEvent({ message: 'message', exception: { values: [{ type: 'exception type 1' }] } }, {}, scope); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Errored); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Errored); }); test('When captureEvent is called without an exception, requestSession status should not be set to Errored', () => { @@ -122,12 +122,12 @@ describe('NodeClient', () => { (client as any)._initSessionFlusher(); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); client.captureEvent({ message: 'message' }, {}, scope); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Ok); }); test('When captureEvent is called with an exception but outside of a request, then requestStatus should not be set', () => { @@ -144,7 +144,7 @@ describe('NodeClient', () => { scope, ); - expect((scope as any)._requestSession).toEqual(undefined); + expect(scope.getRequestSession()).toEqual(undefined); }); test('When captureEvent is called with a transaction, then requestSession status should not be set', () => { @@ -154,26 +154,26 @@ describe('NodeClient', () => { (client as any)._initSessionFlusher(); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); client.captureEvent({ message: 'message', type: 'transaction' }, undefined, scope); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Ok); }); test('When captureEvent is called with an exception but requestHandler is not used, then requestSession status should not be set', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.3' }); const scope = new Scope(); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope.setRequestSession({ status: RequestSessionStatus.Ok }); client.captureEvent( { message: 'message', exception: { values: [{ type: 'exception type 1' }] } }, undefined, scope, ); - const requestSession = (scope as any)._requestSession; - expect(requestSession.status).toEqual(RequestSessionStatus.Ok); + const requestSession = scope.getRequestSession(); + expect(requestSession!.status).toEqual(RequestSessionStatus.Ok); }); }); }); diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 27df4f308e0e..9467fbaa7b33 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -229,7 +229,7 @@ describe('requestHandler', () => { sentryRequestMiddleware(req, res, next); const scope = sentryCore.getCurrentHub().getScope(); - expect((scope as any)._requestSession).toEqual({ status: RequestSessionStatus.Ok }); + expect(scope?.getRequestSession()).toEqual({ status: RequestSessionStatus.Ok }); }); it('autoSessionTracking is disabled, does not set requestSession, when handling a request', () => { @@ -241,7 +241,7 @@ describe('requestHandler', () => { sentryRequestMiddleware(req, res, next); const scope = sentryCore.getCurrentHub().getScope(); - expect((scope as any)._requestSession).toBeUndefined(); + expect(scope?.getRequestSession()).toBeUndefined(); }); it('autoSessionTracking is enabled, calls _captureRequestSession, on response finish', done => { @@ -258,7 +258,7 @@ describe('requestHandler', () => { res.emit('finish'); setImmediate(() => { - expect((scope as any)._requestSession).toEqual({ status: RequestSessionStatus.Ok }); + expect(scope?.getRequestSession()).toEqual({ status: RequestSessionStatus.Ok }); expect(captureRequestSession).toHaveBeenCalled(); done(); }); @@ -276,7 +276,7 @@ describe('requestHandler', () => { res.emit('finish'); setImmediate(() => { - expect((scope as any)._requestSession).toBeUndefined(); + expect(scope?.getRequestSession()).toBeUndefined(); expect(captureRequestSession).not.toHaveBeenCalled(); done(); }); @@ -708,9 +708,9 @@ describe('errorHandler()', () => { jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope?.setRequestSession({ status: RequestSessionStatus.Ok }); sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next); - const requestSession = (scope as any)._requestSession; + const requestSession = scope?.getRequestSession(); expect(requestSession).toEqual({ status: RequestSessionStatus.Ok }); }); @@ -724,9 +724,9 @@ describe('errorHandler()', () => { jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope?.setRequestSession({ status: RequestSessionStatus.Ok }); sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next); - const requestSession = (scope as any)._requestSession; + const requestSession = scope?.getRequestSession(); expect(requestSession).toEqual({ status: RequestSessionStatus.Ok }); }); @@ -742,9 +742,9 @@ describe('errorHandler()', () => { jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub); - (scope as any)._requestSession = { status: RequestSessionStatus.Ok }; + scope?.setRequestSession({ status: RequestSessionStatus.Ok }); sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next); - const requestSession = (scope as any)._requestSession; + const requestSession = scope?.getRequestSession(); expect(requestSession).toEqual({ status: RequestSessionStatus.Crashed }); }); @@ -761,7 +761,7 @@ describe('errorHandler()', () => { jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub); sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next); - const requestSession = (scope as any)._requestSession; + const requestSession = scope?.getRequestSession(); expect(requestSession).toEqual(undefined); }); }); From f1f11de60ea696c3f082f9e4287693d7e1dd6490 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 10:54:45 +0200 Subject: [PATCH 06/10] ref: initSessionFlusher --- packages/node/src/client.ts | 3 ++- packages/node/src/handlers.ts | 3 +-- packages/node/test/client.test.ts | 20 ++++++++++---------- packages/node/test/handlers.test.ts | 6 +++--- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 11a95e37a70a..268bc1e9ff6f 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -13,6 +13,7 @@ import { NodeBackend, NodeOptions } from './backend'; */ export class NodeClient extends BaseClient { protected _sessionFlusher: SessionFlusher | undefined; + /** * Creates a new Node SDK instance. * @param options Configuration options for this SDK. @@ -102,7 +103,7 @@ export class NodeClient extends BaseClient { } /** Method that initialises an instance of SessionFlusher on Client */ - protected _initSessionFlusher(): void { + public initSessionFlusher(): void { const { release, environment } = this._options; if (!release) { logger.warn('Cannot initialise an instance of SessionFlusher if no release is provided!'); diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index ca2d4aa73407..421f5fbfe96c 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -376,8 +376,7 @@ export function requestHandler( // Initialise an instance of SessionFlusher on the client when `autoSessionTracking` is enabled and the // `requestHandler` middleware is used indicating that we are running in SessionAggregates mode if (client && isAutoSessionTrackingEnabled(client)) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); // If Scope contains a Single mode Session, it is removed in favor of using Session Aggregates mode const scope = currentHub.getScope(); diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts index feece0fd7979..1ff6dc73d03e 100644 --- a/packages/node/test/client.test.ts +++ b/packages/node/test/client.test.ts @@ -28,7 +28,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); scope.setRequestSession({ status: RequestSessionStatus.Ok }); @@ -42,7 +42,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); scope.setRequestSession({ status: RequestSessionStatus.Crashed }); @@ -56,7 +56,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); scope.setRequestSession({ status: RequestSessionStatus.Ok }); @@ -70,7 +70,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); @@ -86,7 +86,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); scope.setRequestSession({ status: RequestSessionStatus.Ok }); @@ -104,7 +104,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); scope.setRequestSession({ status: RequestSessionStatus.Ok }); @@ -119,7 +119,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); scope.setRequestSession({ status: RequestSessionStatus.Ok }); @@ -134,7 +134,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '2.2' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); @@ -151,7 +151,7 @@ describe('NodeClient', () => { client = new NodeClient({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.3' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new Scope(); scope.setRequestSession({ status: RequestSessionStatus.Ok }); @@ -186,7 +186,7 @@ describe('flush/close', () => { autoSessionTracking: true, release: '1.1', }); - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); // Clearing interval is important here to ensure that the flush function later on is called by the `client.close()` // not due to the interval running every 60s clearInterval((client as any)._sessionFlusher._intervalId); diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 9467fbaa7b33..4a2b060c887b 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -699,7 +699,7 @@ describe('errorHandler()', () => { client = new NodeClient({ autoSessionTracking: false, release: '3.3' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = sentryCore.getCurrentHub().getScope(); const hub = new Hub(client); @@ -734,7 +734,7 @@ describe('errorHandler()', () => { client = new NodeClient({ autoSessionTracking: true, release: '1.1' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new sentryHub.Scope(); const hub = new Hub(client, scope); @@ -752,7 +752,7 @@ describe('errorHandler()', () => { client = new NodeClient({ autoSessionTracking: true, release: '2.2' }); // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised // by the`requestHandler`) - (client as any)._initSessionFlusher(); + client.initSessionFlusher(); const scope = new sentryHub.Scope(); const hub = new Hub(client, scope); From 0e19077c0b66a874af4a3ea9f4fe306a2d8966ed Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 11:08:33 +0200 Subject: [PATCH 07/10] fix: Linter --- packages/node/src/client.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 268bc1e9ff6f..267c64c360a8 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -91,17 +91,6 @@ export class NodeClient extends BaseClient { return super.close(timeout); } - /** - * @inheritDoc - */ - protected _prepareEvent(event: Event, scope?: Scope, hint?: EventHint): PromiseLike { - event.platform = event.platform || 'node'; - if (this.getOptions().serverName) { - event.server_name = this.getOptions().serverName; - } - return super._prepareEvent(event, scope, hint); - } - /** Method that initialises an instance of SessionFlusher on Client */ public initSessionFlusher(): void { const { release, environment } = this._options; @@ -115,6 +104,17 @@ export class NodeClient extends BaseClient { } } + /** + * @inheritDoc + */ + protected _prepareEvent(event: Event, scope?: Scope, hint?: EventHint): PromiseLike { + event.platform = event.platform || 'node'; + if (this.getOptions().serverName) { + event.server_name = this.getOptions().serverName; + } + return super._prepareEvent(event, scope, hint); + } + /** * Method responsible for capturing/ending a request session by calling `incrementSessionStatusCount` to increment * appropriate session aggregates bucket From f230bd87525e61960f79865d7fe705335e8ce773 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 11:21:58 +0200 Subject: [PATCH 08/10] fix: CR --- packages/browser/src/transports/base.ts | 1 - packages/core/src/request.ts | 3 ++- packages/node/src/transports/base.ts | 1 - packages/types/src/request.ts | 2 +- 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index 7318d46cdfcc..de093c4f33e9 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -15,7 +15,6 @@ const CATEGORY_MAPPING: { event: 'error', transaction: 'transaction', session: 'session', - sessions: 'session', }; /** Base Transport class implementation */ diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts index 212c73b72ab2..f8da800093e9 100644 --- a/packages/core/src/request.ts +++ b/packages/core/src/request.ts @@ -36,7 +36,8 @@ export function sessionToSentryRequest(session: Session | SessionAggregates, api }); let type: SentryRequestType = 'session'; if ('aggregates' in session) { - type = 'sessions'; + // I know this is hacky but we don't want to add `session` to request type since it's never rate limited + type = 'sessions' as SentryRequestType; } const itemHeaders = JSON.stringify({ type, diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts index d4f3b79b4e6d..0cdc369d22bd 100644 --- a/packages/node/src/transports/base.ts +++ b/packages/node/src/transports/base.ts @@ -51,7 +51,6 @@ const CATEGORY_MAPPING: { event: 'error', transaction: 'transaction', session: 'session', - sessions: 'session', }; /** Base Transport class implementation */ diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index 99d2b391bd92..6087c7317b2c 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -1,5 +1,5 @@ /** Possible SentryRequest types that can be used to make a distinction between Sentry features */ -export type SentryRequestType = 'event' | 'transaction' | 'session' | 'sessions'; +export type SentryRequestType = 'event' | 'transaction' | 'session'; /** A generic client request. */ export interface SentryRequest { From 022c427f9408faa3f195c49ae0a1f222c4b99e3f Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 14:36:20 +0200 Subject: [PATCH 09/10] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kamil Ogórek --- packages/core/src/request.ts | 7 ++----- packages/hub/src/session.ts | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts index f8da800093e9..c21d0cbaf583 100644 --- a/packages/core/src/request.ts +++ b/packages/core/src/request.ts @@ -34,11 +34,8 @@ export function sessionToSentryRequest(session: Session | SessionAggregates, api sent_at: new Date().toISOString(), ...(sdkInfo && { sdk: sdkInfo }), }); - let type: SentryRequestType = 'session'; - if ('aggregates' in session) { - // I know this is hacky but we don't want to add `session` to request type since it's never rate limited - type = 'sessions' as SentryRequestType; - } + // I know this is hacky but we don't want to add `session` to request type since it's never rate limited + let type: SentryRequestType = 'aggregates' in session ? ('sessions' as SentryRequestType) : 'session'; const itemHeaders = JSON.stringify({ type, }); diff --git a/packages/hub/src/session.ts b/packages/hub/src/session.ts index 9a6b768ed62b..cea33279126e 100644 --- a/packages/hub/src/session.ts +++ b/packages/hub/src/session.ts @@ -182,7 +182,7 @@ export class SessionFlusher implements SessionFlusherLike { /** Massages the entries in `pendingAggregates` and returns aggregated sessions */ public getSessionAggregates(): SessionAggregates { const aggregates: AggregationCounts[] = Object.keys(this._pendingAggregates).map((key: string) => { - return this._pendingAggregates[parseInt(key)]; + return this._pendingAggregates[key]; }); const sessionAggregates: SessionAggregates = { From 16fd13268607bae9ad6665cc1ab908a74b19a108 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 14 May 2021 14:59:15 +0200 Subject: [PATCH 10/10] fix: Lints --- packages/core/src/request.ts | 2 +- packages/hub/src/session.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts index c21d0cbaf583..8064373083b3 100644 --- a/packages/core/src/request.ts +++ b/packages/core/src/request.ts @@ -35,7 +35,7 @@ export function sessionToSentryRequest(session: Session | SessionAggregates, api ...(sdkInfo && { sdk: sdkInfo }), }); // I know this is hacky but we don't want to add `session` to request type since it's never rate limited - let type: SentryRequestType = 'aggregates' in session ? ('sessions' as SentryRequestType) : 'session'; + const type: SentryRequestType = 'aggregates' in session ? ('sessions' as SentryRequestType) : 'session'; const itemHeaders = JSON.stringify({ type, }); diff --git a/packages/hub/src/session.ts b/packages/hub/src/session.ts index cea33279126e..9a6b768ed62b 100644 --- a/packages/hub/src/session.ts +++ b/packages/hub/src/session.ts @@ -182,7 +182,7 @@ export class SessionFlusher implements SessionFlusherLike { /** Massages the entries in `pendingAggregates` and returns aggregated sessions */ public getSessionAggregates(): SessionAggregates { const aggregates: AggregationCounts[] = Object.keys(this._pendingAggregates).map((key: string) => { - return this._pendingAggregates[key]; + return this._pendingAggregates[parseInt(key)]; }); const sessionAggregates: SessionAggregates = {