Skip to content

feat(various): Apply debug guard to logger everywhere #4698

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

Merged
merged 4 commits into from
Mar 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions packages/angular/src/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { AfterViewInit, Directive, Injectable, Input, NgModule, OnDestroy, OnIni
import { Event, NavigationEnd, NavigationStart, Router } from '@angular/router';
import { getCurrentHub } from '@sentry/browser';
import { Span, Transaction, TransactionContext } from '@sentry/types';
import { getGlobalObject, logger, stripUrlQueryAndFragment, timestampWithMs } from '@sentry/utils';
import { getGlobalObject, isDebugBuild, logger, stripUrlQueryAndFragment, timestampWithMs } from '@sentry/utils';
import { Observable, Subscription } from 'rxjs';
import { filter, tap } from 'rxjs/operators';

Expand Down Expand Up @@ -63,7 +63,8 @@ export class TraceService implements OnDestroy {
filter(event => event instanceof NavigationStart),
tap(event => {
if (!instrumentationInitialized) {
logger.error('Angular integration has tracing enabled, but Tracing integration is not configured');
isDebugBuild() &&
logger.error('Angular integration has tracing enabled, but Tracing integration is not configured');
return;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/browser/src/client.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { BaseClient, Scope, SDK_VERSION } from '@sentry/core';
import { Event, EventHint } from '@sentry/types';
import { getGlobalObject, logger } from '@sentry/utils';
import { getGlobalObject, isDebugBuild, logger } from '@sentry/utils';

import { BrowserBackend, BrowserOptions } from './backend';
import { injectReportDialog, ReportDialogOptions } from './helpers';
Expand Down Expand Up @@ -47,7 +47,7 @@ export class BrowserClient extends BaseClient<BrowserBackend, BrowserOptions> {
}

if (!this._isEnabled()) {
logger.error('Trying to call showReportDialog with Sentry Client disabled');
isDebugBuild() && logger.error('Trying to call showReportDialog with Sentry Client disabled');
return;
}

Expand Down
8 changes: 2 additions & 6 deletions packages/browser/src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,16 +192,12 @@ export function injectReportDialog(options: ReportDialogOptions = {}): void {
}

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

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

Expand Down
4 changes: 2 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,7 @@ 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.');
isDebugBuild() && logger.warn('Event dropped due to being a duplicate of previously captured event.');
return null;
}
} catch (_oO) {
Expand Down
4 changes: 1 addition & 3 deletions packages/browser/src/integrations/globalhandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,7 @@ function _enhanceEventWithInitialFrame(event: Event, url: any, line: any, column
}

function globalHandlerLog(type: string): void {
if (isDebugBuild()) {
logger.log(`Global Handler attached: ${type}`);
}
isDebugBuild() && logger.log(`Global Handler attached: ${type}`);
}

function addMechanismAndCapture(hub: Hub, error: EventHint['originalException'], event: Event, type: string): void {
Expand Down
12 changes: 3 additions & 9 deletions packages/browser/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,7 @@ export function flush(timeout?: number): PromiseLike<boolean> {
if (client) {
return client.flush(timeout);
}
if (isDebugBuild()) {
logger.warn('Cannot flush events. No client defined.');
}
isDebugBuild() && logger.warn('Cannot flush events. No client defined.');
return resolvedSyncPromise(false);
}

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

Expand Down Expand Up @@ -212,9 +208,7 @@ function startSessionTracking(): void {
const document = window.document;

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

Expand Down
13 changes: 7 additions & 6 deletions packages/browser/src/transports/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ 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}`);
isDebugBuild() && logger.log(`Adding outcome: ${key}`);
this._outcomes[key] = (this._outcomes[key] ?? 0) + 1;
}

Expand All @@ -122,11 +122,11 @@ export abstract class BaseTransport implements Transport {

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

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

const url = getEnvelopeEndpointWithUrlEncodedAuth(this._api.dsn, this._api.tunnel);

Expand All @@ -144,7 +144,7 @@ export abstract class BaseTransport implements Transport {
try {
sendReport(url, serializeEnvelope(envelope));
} catch (e) {
logger.error(e);
isDebugBuild() && logger.error(e);
}
}

Expand All @@ -170,8 +170,9 @@ export abstract class BaseTransport implements Transport {
* https://developer.mozilla.org/en-US/docs/Web/API/Headers/get
*/
const limited = this._handleRateLimit(headers);
if (limited && isDebugBuild()) {
logger.warn(`Too many ${requestType} requests, backing off until: ${this._disabledUntil(requestType)}`);
if (limited) {
isDebugBuild() &&
logger.warn(`Too many ${requestType} requests, backing off until: ${this._disabledUntil(requestType)}`);
}

if (status === 'success') {
Expand Down
3 changes: 1 addition & 2 deletions packages/browser/src/transports/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,8 @@ export function getNativeFetchImplementation(): FetchImpl {
}
document.head.removeChild(sandbox);
} catch (e) {
if (isDebugBuild()) {
isDebugBuild() &&
logger.warn('Could not create sandbox iframe for pure fetch check, bailing to window.fetch: ', e);
}
}
}

Expand Down
14 changes: 4 additions & 10 deletions packages/core/src/basebackend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export abstract class BaseBackend<O extends Options> implements Backend {
public constructor(options: O) {
this._options = options;
if (!this._options.dsn) {
logger.warn('No DSN provided, backend will not do anything.');
isDebugBuild() && logger.warn('No DSN provided, backend will not do anything.');
}
this._transport = this._setupTransport();
}
Expand All @@ -92,9 +92,7 @@ export abstract class BaseBackend<O extends Options> implements Backend {
*/
public sendEvent(event: Event): void {
void this._transport.sendEvent(event).then(null, reason => {
if (isDebugBuild()) {
logger.error('Error while sending event:', reason);
}
isDebugBuild() && logger.error('Error while sending event:', reason);
});
}

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

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

Expand Down
16 changes: 6 additions & 10 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ 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);
isDebugBuild() && logger.log(ALREADY_SEEN_ERROR);
return;
}

Expand Down Expand Up @@ -153,7 +153,7 @@ 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);
isDebugBuild() && logger.log(ALREADY_SEEN_ERROR);
return;
}

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

if (!(typeof session.release === 'string')) {
if (isDebugBuild()) {
logger.warn('Discarded session because of missing or non-string release');
}
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 @@ -248,7 +244,7 @@ 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`);
isDebugBuild() && logger.warn(`Cannot retrieve integration ${integration.id} from the current Client`);
return null;
}
}
Expand Down Expand Up @@ -507,7 +503,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
return finalEvent.event_id;
},
reason => {
logger.error(reason);
isDebugBuild() && logger.error(reason);
return undefined;
},
);
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/integration.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { addGlobalEventProcessor, getCurrentHub } from '@sentry/hub';
import { Integration, Options } from '@sentry/types';
import { addNonEnumerableProperty, logger } from '@sentry/utils';
import { addNonEnumerableProperty, isDebugBuild, logger } from '@sentry/utils';

export const installedIntegrations: string[] = [];

Expand Down Expand Up @@ -59,7 +59,7 @@ export function setupIntegration(integration: Integration): void {
}
integration.setupOnce(addGlobalEventProcessor, getCurrentHub);
installedIntegrations.push(integration.name);
logger.log(`Integration installed: ${integration.name}`);
isDebugBuild() && logger.log(`Integration installed: ${integration.name}`);
}

/**
Expand Down
20 changes: 6 additions & 14 deletions packages/core/src/integrations/inboundfilters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,37 +64,33 @@ export class InboundFilters implements Integration {
/** JSDoc */
private _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean {
if (this._isSentryError(event, options)) {
if (isDebugBuild()) {
isDebugBuild() &&
logger.warn(`Event dropped due to being internal Sentry Error.\nEvent: ${getEventDescription(event)}`);
}
return true;
}
if (this._isIgnoredError(event, options)) {
if (isDebugBuild()) {
isDebugBuild() &&
logger.warn(
`Event dropped due to being matched by \`ignoreErrors\` option.\nEvent: ${getEventDescription(event)}`,
);
}
return true;
}
if (this._isDeniedUrl(event, options)) {
if (isDebugBuild()) {
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)) {
if (isDebugBuild()) {
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 @@ -187,9 +183,7 @@ export class InboundFilters implements Integration {
const { type = '', value = '' } = (event.exception.values && event.exception.values[0]) || {};
return [`${value}`, `${type}: ${value}`];
} catch (oO) {
if (isDebugBuild()) {
logger.error(`Cannot extract message for event ${getEventDescription(event)}`);
}
isDebugBuild() && logger.error(`Cannot extract message for event ${getEventDescription(event)}`);
return [];
}
}
Expand Down Expand Up @@ -224,9 +218,7 @@ export class InboundFilters implements Integration {
}
return frames ? this._getLastValidUrl(frames) : null;
} catch (oO) {
if (isDebugBuild()) {
logger.error(`Cannot extract url for event ${getEventDescription(event)}`);
}
isDebugBuild() && logger.error(`Cannot extract url for event ${getEventDescription(event)}`);
return null;
}
}
Expand Down
10 changes: 8 additions & 2 deletions 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,7 +14,13 @@ 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) {
logger.enable();
if (isDebugBuild()) {
logger.enable();
} else {
// use `console.warn` rather than `logger.warn` since by non-debug bundles have all `logger.x` statements stripped
// eslint-disable-next-line no-console
console.warn('[Sentry] Cannot initialize SDK with `debug` option using a non-debug bundle.');
}
}
const hub = getCurrentHub();
const scope = hub.getScope();
Expand Down
Loading