Skip to content

ref: Refactorings for the logger to be less bloaty #4285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
09b2736
ref: Refactorings for the logger to be less bloaty
mitsuhiko Dec 14, 2021
657da5e
feat: disable the logger on disabled debug mode
mitsuhiko Dec 14, 2021
ae28044
feat: Disable a lot of debug logs in non debug builds
mitsuhiko Dec 14, 2021
77ca75e
fix: lint
mitsuhiko Dec 14, 2021
3d1c690
Merge branch 'master' into feature/smol-logger
mitsuhiko Dec 15, 2021
70c6f2b
Merge branch 'master' into feature/smol-logger
mitsuhiko Dec 15, 2021
4e493c7
ref: slightly less weird code for console sandbox
mitsuhiko Dec 15, 2021
e861b7a
fix: copy all console methods over
mitsuhiko Dec 15, 2021
3035368
ref: simplify console sandbox
mitsuhiko Dec 15, 2021
4684022
fix: lint
mitsuhiko Dec 15, 2021
d689fb7
Merge branch 'master' into feature/smol-logger
mitsuhiko Dec 15, 2021
19db7d9
Merge branch 'master' into feature/smol-logger
mitsuhiko Dec 15, 2021
b3cef5a
Merge branch 'master' into feature/smol-logger
mitsuhiko Dec 15, 2021
372359f
Merge branch 'master' into feature/smol-logger
mitsuhiko Dec 16, 2021
fa2b414
Merge branch 'feature/smol-logger' of github.com:getsentry/sentry-jav…
mitsuhiko Dec 16, 2021
27fb841
Merge branch 'master' into feature/smol-logger
mitsuhiko Dec 16, 2021
d40f4d2
Merge branch 'master' into feature/smol-logger
mitsuhiko Dec 17, 2021
dbc96f0
fix: remember integrations globally
mitsuhiko Dec 17, 2021
a70ac85
ref: try to refactor the use of global singletons
mitsuhiko Dec 17, 2021
9f52f2e
fix: getGlobalSingleton
mitsuhiko Dec 17, 2021
8a9f2d0
fix: need to export a const for another module
mitsuhiko Dec 17, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/browser/src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
addNonEnumerableProperty,
getGlobalObject,
getOriginalFunction,
isDebugBuild,
logger,
markFunctionWrapped,
} from '@sentry/utils';
Expand Down Expand Up @@ -191,12 +192,16 @@ export function injectReportDialog(options: ReportDialogOptions = {}): void {
}

if (!options.eventId) {
logger.error(`Missing eventId option in showReportDialog call`);
if (isDebugBuild()) {
logger.error(`Missing eventId option in showReportDialog call`);
}
return;
}

if (!options.dsn) {
logger.error(`Missing dsn option in showReportDialog call`);
if (isDebugBuild()) {
logger.error(`Missing dsn option in showReportDialog call`);
}
return;
}

Expand Down
6 changes: 4 additions & 2 deletions packages/browser/src/integrations/dedupe.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Event, EventProcessor, Exception, Hub, Integration, StackFrame } from '@sentry/types';
import { logger } from '@sentry/utils';
import { isDebugBuild, logger } from '@sentry/utils';

/** Deduplication filter */
export class Dedupe implements Integration {
Expand Down Expand Up @@ -28,7 +28,9 @@ export class Dedupe implements Integration {
// Juuust in case something goes wrong
try {
if (_shouldDropEvent(currentEvent, self._previousEvent)) {
logger.warn(`Event dropped due to being a duplicate of previously captured event.`);
if (isDebugBuild()) {
logger.warn(`Event dropped due to being a duplicate of previously captured event.`);
}
return null;
}
} catch (_oO) {
Expand Down
14 changes: 10 additions & 4 deletions packages/browser/src/sdk.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getCurrentHub, initAndBind, Integrations as CoreIntegrations } from '@sentry/core';
import { addInstrumentationHandler, getGlobalObject, logger, SyncPromise } from '@sentry/utils';
import { addInstrumentationHandler, getGlobalObject, isDebugBuild, logger, SyncPromise } from '@sentry/utils';

import { BrowserOptions } from './backend';
import { BrowserClient } from './client';
Expand Down Expand Up @@ -161,7 +161,9 @@ export function flush(timeout?: number): PromiseLike<boolean> {
if (client) {
return client.flush(timeout);
}
logger.warn('Cannot flush events. No client defined.');
if (isDebugBuild()) {
logger.warn('Cannot flush events. No client defined.');
}
return SyncPromise.resolve(false);
}

Expand All @@ -178,7 +180,9 @@ export function close(timeout?: number): PromiseLike<boolean> {
if (client) {
return client.close(timeout);
}
logger.warn('Cannot flush events and disable SDK. No client defined.');
if (isDebugBuild()) {
logger.warn('Cannot flush events and disable SDK. No client defined.');
}
return SyncPromise.resolve(false);
}

Expand All @@ -202,7 +206,9 @@ function startSessionTracking(): void {
const document = window.document;

if (typeof document === 'undefined') {
logger.warn('Session tracking in non-browser environment with @sentry/browser is not supported.');
if (isDebugBuild()) {
logger.warn('Session tracking in non-browser environment with @sentry/browser is not supported.');
}
return;
}

Expand Down
17 changes: 13 additions & 4 deletions packages/browser/src/transports/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
dateTimestampInSeconds,
eventStatusFromHttpCode,
getGlobalObject,
isDebugBuild,
logger,
makePromiseBuffer,
parseRetryAfterHeader,
Expand Down Expand Up @@ -91,7 +92,9 @@ export abstract class BaseTransport implements Transport {
// A correct type for map-based implementation if we want to go that route
// would be `Partial<Record<SentryRequestType, Partial<Record<Outcome, number>>>>`
const key = `${requestTypeToCategory(category)}:${reason}`;
logger.log(`Adding outcome: ${key}`);
if (isDebugBuild()) {
logger.log(`Adding outcome: ${key}`);
}
this._outcomes[key] = (this._outcomes[key] ?? 0) + 1;
}

Expand All @@ -108,11 +111,15 @@ export abstract class BaseTransport implements Transport {

// Nothing to send
if (!Object.keys(outcomes).length) {
logger.log('No outcomes to flush');
if (isDebugBuild()) {
logger.log('No outcomes to flush');
}
return;
}

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

const url = getEnvelopeEndpointWithUrlEncodedAuth(this._api.dsn, this._api.tunnel);
// Envelope header is required to be at least an empty object
Expand Down Expand Up @@ -163,7 +170,9 @@ export abstract class BaseTransport implements Transport {
*/
const limited = this._handleRateLimit(headers);
if (limited)
logger.warn(`Too many ${requestType} requests, backing off until: ${this._disabledUntil(requestType)}`);
if (isDebugBuild()) {
logger.warn(`Too many ${requestType} requests, backing off until: ${this._disabledUntil(requestType)}`);
}

if (status === 'success') {
resolve({ status });
Expand Down
6 changes: 4 additions & 2 deletions packages/browser/src/transports/utils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { forget, getGlobalObject, isNativeFetch, logger, supportsFetch } from '@sentry/utils';
import { forget, getGlobalObject, isDebugBuild, isNativeFetch, logger, supportsFetch } from '@sentry/utils';

const global = getGlobalObject<Window>();
let cachedFetchImpl: FetchImpl;
Expand Down Expand Up @@ -69,7 +69,9 @@ export function getNativeFetchImplementation(): FetchImpl {
}
document.head.removeChild(sandbox);
} catch (e) {
logger.warn('Could not create sandbox iframe for pure fetch check, bailing to window.fetch: ', e);
if (isDebugBuild()) {
logger.warn('Could not create sandbox iframe for pure fetch check, bailing to window.fetch: ', e);
}
}
}

Expand Down
16 changes: 11 additions & 5 deletions packages/core/src/basebackend.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Event, EventHint, Options, Session, SeverityLevel, Transport } from '@sentry/types';
import { logger, SentryError } from '@sentry/utils';
import { isDebugBuild, logger, SentryError } from '@sentry/utils';

import { NoopTransport } from './transports/noop';

Expand Down Expand Up @@ -66,7 +66,7 @@ export abstract class BaseBackend<O extends Options> implements Backend {
/** Creates a new backend instance. */
public constructor(options: O) {
this._options = options;
if (!this._options.dsn) {
if (isDebugBuild() && !this._options.dsn) {
logger.warn('No DSN provided, backend will not do anything.');
}
this._transport = this._setupTransport();
Expand All @@ -92,7 +92,9 @@ export abstract class BaseBackend<O extends Options> implements Backend {
*/
public sendEvent(event: Event): void {
void this._transport.sendEvent(event).then(null, reason => {
logger.error(`Error while sending event: ${reason}`);
if (isDebugBuild()) {
logger.error(`Error while sending event: ${reason}`);
}
});
}

Expand All @@ -101,12 +103,16 @@ export abstract class BaseBackend<O extends Options> implements Backend {
*/
public sendSession(session: Session): void {
if (!this._transport.sendSession) {
logger.warn("Dropping session because custom transport doesn't implement sendSession");
if (isDebugBuild()) {
logger.warn("Dropping session because custom transport doesn't implement sendSession");
}
return;
}

void this._transport.sendSession(session).then(null, reason => {
logger.error(`Error while sending session: ${reason}`);
if (isDebugBuild()) {
logger.error(`Error while sending session: ${reason}`);
}
});
}

Expand Down
21 changes: 16 additions & 5 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
checkOrSetAlreadyCaught,
dateTimestampInSeconds,
Dsn,
isDebugBuild,
isPlainObject,
isPrimitive,
isThenable,
Expand Down Expand Up @@ -104,7 +105,9 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined {
// ensure we haven't captured this very object before
if (checkOrSetAlreadyCaught(exception)) {
logger.log(ALREADY_SEEN_ERROR);
if (isDebugBuild()) {
logger.log(ALREADY_SEEN_ERROR);
}
return;
}

Expand Down Expand Up @@ -149,7 +152,9 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined {
// ensure we haven't captured this very object before
if (hint && hint.originalException && checkOrSetAlreadyCaught(hint.originalException)) {
logger.log(ALREADY_SEEN_ERROR);
if (isDebugBuild()) {
logger.log(ALREADY_SEEN_ERROR);
}
return;
}

Expand All @@ -169,12 +174,16 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
*/
public captureSession(session: Session): void {
if (!this._isEnabled()) {
logger.warn('SDK not enabled, will not capture session.');
if (isDebugBuild()) {
logger.warn('SDK not enabled, will not capture session.');
}
return;
}

if (!(typeof session.release === 'string')) {
logger.warn('Discarded session because of missing or non-string release');
if (isDebugBuild()) {
logger.warn('Discarded session because of missing or non-string release');
}
} else {
this._sendSession(session);
// After sending, we set init false to indicate it's not the first occurrence
Expand Down Expand Up @@ -240,7 +249,9 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
try {
return (this._integrations[integration.id] as T) || null;
} catch (_oO) {
logger.warn(`Cannot retrieve integration ${integration.id} from the current Client`);
if (isDebugBuild()) {
logger.warn(`Cannot retrieve integration ${integration.id} from the current Client`);
}
return null;
}
}
Expand Down
16 changes: 11 additions & 5 deletions packages/core/src/integration.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { addGlobalEventProcessor, getCurrentHub } from '@sentry/hub';
import { Integration, Options } from '@sentry/types';
import { addNonEnumerableProperty, logger } from '@sentry/utils';
import { addNonEnumerableProperty, getGlobalSingleton, isDebugBuild, logger } from '@sentry/utils';

export const installedIntegrations: string[] = [];
// we have to remember integrations globally on the __SENTRY__ object in case
// sentry is bundled twice. In that case integrations would patch over themselves.
// this is problematic because it's unclear if sentry versions will agree on this
// behavior. There are likely to be better ways to accomplish this.
export const onceInitializedIntegrations: Array<string> = getGlobalSingleton('_integrations', () => []);

/** Map of integrations assigned to a client */
export type IntegrationIndex = {
Expand Down Expand Up @@ -54,12 +58,14 @@ export function getIntegrationsToSetup(options: Options): Integration[] {

/** Setup given integration */
export function setupIntegration(integration: Integration): void {
if (installedIntegrations.indexOf(integration.name) !== -1) {
if (onceInitializedIntegrations.indexOf(integration.name) >= 0) {
return;
}
integration.setupOnce(addGlobalEventProcessor, getCurrentHub);
installedIntegrations.push(integration.name);
logger.log(`Integration installed: ${integration.name}`);
onceInitializedIntegrations.push(integration.name);
if (isDebugBuild()) {
logger.log(`Integration installed: ${integration.name}`);
}
}

/**
Expand Down
46 changes: 29 additions & 17 deletions packages/core/src/integrations/inboundfilters.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { addGlobalEventProcessor, getCurrentHub } from '@sentry/hub';
import { Event, Integration, StackFrame } from '@sentry/types';
import { getEventDescription, isMatchingPattern, logger } from '@sentry/utils';
import { getEventDescription, isDebugBuild, isMatchingPattern, logger } from '@sentry/utils';

// "Script error." is hard coded into browsers for errors that it can't read.
// this is the result of a script being pulled in from an external domain and CORS.
Expand Down Expand Up @@ -64,29 +64,37 @@ export class InboundFilters implements Integration {
/** JSDoc */
private _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean {
if (this._isSentryError(event, options)) {
logger.warn(`Event dropped due to being internal Sentry Error.\nEvent: ${getEventDescription(event)}`);
if (isDebugBuild()) {
logger.warn(`Event dropped due to being internal Sentry Error.\nEvent: ${getEventDescription(event)}`);
}
return true;
}
if (this._isIgnoredError(event, options)) {
logger.warn(
`Event dropped due to being matched by \`ignoreErrors\` option.\nEvent: ${getEventDescription(event)}`,
);
if (isDebugBuild()) {
logger.warn(
`Event dropped due to being matched by \`ignoreErrors\` option.\nEvent: ${getEventDescription(event)}`,
);
}
return true;
}
if (this._isDeniedUrl(event, options)) {
logger.warn(
`Event dropped due to being matched by \`denyUrls\` option.\nEvent: ${getEventDescription(
event,
)}.\nUrl: ${this._getEventFilterUrl(event)}`,
);
if (isDebugBuild()) {
logger.warn(
`Event dropped due to being matched by \`denyUrls\` option.\nEvent: ${getEventDescription(
event,
)}.\nUrl: ${this._getEventFilterUrl(event)}`,
);
}
return true;
}
if (!this._isAllowedUrl(event, options)) {
logger.warn(
`Event dropped due to not being matched by \`allowUrls\` option.\nEvent: ${getEventDescription(
event,
)}.\nUrl: ${this._getEventFilterUrl(event)}`,
);
if (isDebugBuild()) {
logger.warn(
`Event dropped due to not being matched by \`allowUrls\` option.\nEvent: ${getEventDescription(
event,
)}.\nUrl: ${this._getEventFilterUrl(event)}`,
);
}
return true;
}
return false;
Expand Down Expand Up @@ -179,7 +187,9 @@ export class InboundFilters implements Integration {
const { type = '', value = '' } = (event.exception.values && event.exception.values[0]) || {};
return [`${value}`, `${type}: ${value}`];
} catch (oO) {
logger.error(`Cannot extract message for event ${getEventDescription(event)}`);
if (isDebugBuild()) {
logger.error(`Cannot extract message for event ${getEventDescription(event)}`);
}
return [];
}
}
Expand Down Expand Up @@ -214,7 +224,9 @@ export class InboundFilters implements Integration {
}
return frames ? this._getLastValidUrl(frames) : null;
} catch (oO) {
logger.error(`Cannot extract url for event ${getEventDescription(event)}`);
if (isDebugBuild()) {
logger.error(`Cannot extract url for event ${getEventDescription(event)}`);
}
return null;
}
}
Expand Down
6 changes: 5 additions & 1 deletion packages/core/src/sdk.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { getCurrentHub } from '@sentry/hub';
import { Client, Options } from '@sentry/types';
import { logger } from '@sentry/utils';
import { isDebugBuild, logger } from '@sentry/utils';

/** A class object that can instantiate Client objects. */
export type ClientClass<F extends Client, O extends Options> = new (options: O) => F;
Expand All @@ -14,6 +14,10 @@ export type ClientClass<F extends Client, O extends Options> = new (options: O)
*/
export function initAndBind<F extends Client, O extends Options>(clientClass: ClientClass<F, O>, options: O): void {
if (options.debug === true) {
if (!isDebugBuild()) {
// eslint-disable-next-line no-console
console.warn('warning: non debug Sentry SDK loaded, debug mode unavailable!');
}
logger.enable();
}
const hub = getCurrentHub();
Expand Down
Loading