Skip to content

ref(utils): small gains attempt #4324

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

ref(utils): small gains attempt #4324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be7938b
ref(utils): smaller loop
JonasBa Dec 16, 2021
f584d93
ref(utils): move some time utils
JonasBa Dec 16, 2021
90ce852
ref(utils): use the util
JonasBa Dec 16, 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
3 changes: 2 additions & 1 deletion packages/browser/src/transports/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
logger,
parseRetryAfterHeader,
PromiseBuffer,
secToMs,
SentryError,
} from '@sentry/utils';

Expand Down Expand Up @@ -209,7 +210,7 @@ export abstract class BaseTransport implements Transport {
for (const limit of rlHeader.trim().split(',')) {
const parameters = limit.split(':', 2);
const headerDelay = parseInt(parameters[0], 10);
const delay = (!isNaN(headerDelay) ? headerDelay : 60) * 1000; // 60sec default
const delay = secToMs(!isNaN(headerDelay) ? headerDelay : 60); // 60sec default
for (const category of parameters[1].split(';')) {
this._rateLimits[category || 'all'] = new Date(now + delay);
}
Expand Down
13 changes: 5 additions & 8 deletions packages/ember/addon/instance-initializers/sentry-performance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as Sentry from '@sentry/browser';
import { Span, Transaction, Integration } from '@sentry/types';
import { EmberRunQueues } from '@ember/runloop/-private/types';
import { getActiveTransaction } from '..';
import { browserPerformanceTimeOrigin, getGlobalObject, timestampWithMs } from '@sentry/utils';
import { browserPerformanceTimeOrigin, getGlobalObject, timestampWithMs, secToMs, msToSec } from '@sentry/utils';
import { macroCondition, isTesting, getOwnConfig } from '@embroider/macros';
import { EmberSentryConfig, GlobalConfig, OwnConfig } from '../types';

Expand All @@ -29,10 +29,7 @@ export function initialize(appInstance: ApplicationInstance): void {
}

function getBackburner() {
if (run.backburner) {
return run.backburner;
}
return _backburner;
return run.backburner || _backburner;
}

function getTransitionInformation(transition: any, router: any) {
Expand Down Expand Up @@ -170,7 +167,7 @@ function _instrumentEmberRunloop(config: EmberSentryConfig) {
const now = timestampWithMs();
const minQueueDuration = minimumRunloopQueueDuration ?? 5;

if ((now - currentQueueStart) * 1000 >= minQueueDuration) {
if (secToMs(now - currentQueueStart) >= minQueueDuration) {
activeTransaction
?.startChild({
op: `ui.ember.runloop.${queue}`,
Expand Down Expand Up @@ -326,8 +323,8 @@ function _instrumentInitialLoad(config: EmberSentryConfig) {
const measures = performance.getEntriesByName(measureName);
const measure = measures[0];

const startTimestamp = (measure.startTime + browserPerformanceTimeOrigin!) / 1000;
const endTimestamp = startTimestamp + measure.duration / 1000;
const startTimestamp = msToSec(measure.startTime + browserPerformanceTimeOrigin!);
const endTimestamp = startTimestamp + msToSec(measure.duration);

const transaction = getActiveTransaction();
const span = transaction?.startChild({
Expand Down
11 changes: 9 additions & 2 deletions packages/node/src/transports/base/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,14 @@ import {
Transport,
TransportOptions,
} from '@sentry/types';
import { eventStatusFromHttpCode, logger, parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sentry/utils';
import {
eventStatusFromHttpCode,
logger,
parseRetryAfterHeader,
PromiseBuffer,
secToMs,
SentryError,
} from '@sentry/utils';
import * as fs from 'fs';
import * as http from 'http';
import * as https from 'https';
Expand Down Expand Up @@ -161,7 +168,7 @@ export abstract class BaseTransport implements Transport {
for (const limit of rlHeader.trim().split(',')) {
const parameters = limit.split(':', 2);
const headerDelay = parseInt(parameters[0], 10);
const delay = (!isNaN(headerDelay) ? headerDelay : 60) * 1000; // 60sec default
const delay = secToMs(!isNaN(headerDelay) ? headerDelay : 60); // 60sec default
for (const category of (parameters[1] && parameters[1].split(';')) || ['all']) {
// categoriesAllowed is added here to ensure we are only storing rate limits for categories we support in this
// sdk and any categories that are not supported will not be added redundantly to the rateLimits object
Expand Down
4 changes: 2 additions & 2 deletions packages/serverless/src/awslambda.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from '@sentry/node';
import { extractTraceparentData } from '@sentry/tracing';
import { Integration } from '@sentry/types';
import { isString, logger } from '@sentry/utils';
import { isString, logger, msToSec } from '@sentry/utils';
// NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil
// eslint-disable-next-line import/no-unresolved
import { Context, Handler } from 'aws-lambda';
Expand Down Expand Up @@ -257,7 +257,7 @@ export function wrapHandler<TEvent, TResult>(
context.callbackWaitsForEmptyEventLoop = options.callbackWaitsForEmptyEventLoop;

// In seconds. You cannot go any more granular than this in AWS Lambda.
const configuredTimeout = Math.ceil(tryGetRemainingTimeInMillis(context) / 1000);
const configuredTimeout = msToSec(Math.ceil(tryGetRemainingTimeInMillis(context)));
const configuredTimeoutMinutes = Math.floor(configuredTimeout / 60);
const configuredTimeoutSeconds = configuredTimeout % 60;

Expand Down
4 changes: 2 additions & 2 deletions packages/tracing/src/browser/browsertracing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { Hub } from '@sentry/hub';
import { EventProcessor, Integration, Transaction, TransactionContext } from '@sentry/types';
import { getGlobalObject, logger } from '@sentry/utils';
import { getGlobalObject, logger, secToMs } from '@sentry/utils';

import { startIdleTransaction } from '../hubextensions';
import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../idletransaction';
import { extractTraceparentData, secToMs } from '../utils';
import { extractTraceparentData } from '../utils';
import { registerBackgroundTabDetection } from './backgroundtab';
import { MetricsInstrumentation } from './metrics';
import {
Expand Down
10 changes: 8 additions & 2 deletions packages/tracing/src/browser/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
/* eslint-disable max-lines */
/* eslint-disable @typescript-eslint/no-explicit-any */
import { Measurements, SpanContext } from '@sentry/types';
import { browserPerformanceTimeOrigin, getGlobalObject, htmlTreeAsString, isNodeEnv, logger } from '@sentry/utils';
import {
browserPerformanceTimeOrigin,
getGlobalObject,
htmlTreeAsString,
isNodeEnv,
logger,
msToSec,
} from '@sentry/utils';

import { Span } from '../span';
import { Transaction } from '../transaction';
import { msToSec } from '../utils';
import { getCLS, LayoutShift } from './web-vitals/getCLS';
import { getFID } from './web-vitals/getFID';
import { getLCP, LargestContentfulPaint } from './web-vitals/getLCP';
Expand Down
4 changes: 2 additions & 2 deletions packages/tracing/src/idletransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Hub } from '@sentry/hub';
import { TransactionContext } from '@sentry/types';
import { logger, timestampWithMs } from '@sentry/utils';
import { logger, msToSec, timestampWithMs } from '@sentry/utils';

import { FINISH_REASON_TAG, IDLE_TRANSACTION_FINISH_REASONS } from './constants';
import { Span, SpanRecorder } from './span';
Expand Down Expand Up @@ -219,7 +219,7 @@ export class IdleTransaction extends Transaction {
const timeout = this._idleTimeout;
// We need to add the timeout here to have the real endtimestamp of the transaction
// Remember timestampWithMs is in seconds, timeout is in ms
const end = timestampWithMs() + timeout / 1000;
const end = timestampWithMs() + msToSec(timeout);

setTimeout(() => {
if (!this._finished) {
Expand Down
16 changes: 0 additions & 16 deletions packages/tracing/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,5 @@ export function getActiveTransaction<T extends Transaction>(maybeHub?: Hub): T |
return scope && (scope.getTransaction() as T | undefined);
}

/**
* Converts from milliseconds to seconds
* @param time time in ms
*/
export function msToSec(time: number): number {
return time / 1000;
}

/**
* Converts from seconds to milliseconds
* @param time time in seconds
*/
export function secToMs(time: number): number {
return time * 1000;
}

// so it can be used in manual instrumentation without necessitating a hard dependency on @sentry/utils
export { stripUrlQueryAndFragment } from '@sentry/utils';
15 changes: 10 additions & 5 deletions packages/utils/src/misc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Event, Exception, Mechanism, StackFrame } from '@sentry/types';
import { getGlobalObject } from './global';
import { addNonEnumerableProperty } from './object';
import { snipLine } from './string';
import { secToMs } from './time';

/**
* Extended Window interface that allows for Crypto API usage in IE browsers
Expand Down Expand Up @@ -41,9 +42,13 @@ export function uuid4(): string {
return v;
};

return (
pad(arr[0]) + pad(arr[1]) + pad(arr[2]) + pad(arr[3]) + pad(arr[4]) + pad(arr[5]) + pad(arr[6]) + pad(arr[7])
);
let i = 0;
let str = pad(arr[0]);

// eslint-disable-next-line no-plusplus
while (i++ < 7) str += pad(arr[i]);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there is performance implications from un-unrolling the loop here, esp since we use this heavily for tracing

Copy link
Member Author

Choose a reason for hiding this comment

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

@AbhiPrasad let me benchmark this

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, about 5% drop https://jsbench.me/pmkx990pc4/1, I think I'll just drop this PR, negligible bundle size wins.


return str;
}
// http://stackoverflow.com/questions/105034/how-to-create-a-guid-uuid-in-javascript/2117523#2117523
return 'xxxxxxxxxxxx4xxxyxxxxxxxxxxxxxxx'.replace(/[xy]/g, c => {
Expand Down Expand Up @@ -189,7 +194,7 @@ export function parseSemver(input: string): SemVer {
};
}

const defaultRetryAfter = 60 * 1000; // 60 seconds
const defaultRetryAfter = secToMs(60); // 60 seconds

/**
* Extracts Retry-After value from the request header or returns default value
Expand All @@ -203,7 +208,7 @@ export function parseRetryAfterHeader(now: number, header?: string | number | nu

const headerDelay = parseInt(`${header}`, 10);
if (!isNaN(headerDelay)) {
return headerDelay * 1000;
return secToMs(headerDelay);
}

const headerDate = Date.parse(`${header}`);
Expand Down
20 changes: 18 additions & 2 deletions packages/utils/src/time.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ interface TimestampSource {
* is more obvious to explain "why does my span have negative duration" than "why my spans have zero duration".
*/
const dateTimestampSource: TimestampSource = {
nowSeconds: () => Date.now() / 1000,
nowSeconds: () => msToSec(Date.now()),
Copy link
Member

Choose a reason for hiding this comment

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

We can prob just stop using the nowSeconds pattern as well, idk why it exists. A quick patch I have saved from before.

diff --git a/packages/utils/src/time.ts b/packages/utils/src/time.ts
index 932fffc9..ca59433a 100644
--- a/packages/utils/src/time.ts
+++ b/packages/utils/src/time.ts
@@ -1,24 +1,6 @@
 import { getGlobalObject } from './global';
 import { dynamicRequire, isNodeEnv } from './node';
 
-/**
- * An object that can return the current timestamp in seconds since the UNIX epoch.
- */
-interface TimestampSource {
-  nowSeconds(): number;
-}
-
-/**
- * A TimestampSource implementation for environments that do not support the Performance Web API natively.
- *
- * Note that this TimestampSource does not use a monotonic clock. A call to `nowSeconds` may return a timestamp earlier
- * than a previously returned value. We do not try to emulate a monotonic behavior in order to facilitate debugging. It
- * is more obvious to explain "why does my span have negative duration" than "why my spans have zero duration".
- */
-const dateTimestampSource: TimestampSource = {
-  nowSeconds: () => Date.now() / 1000,
-};
-
 /**
  * A partial definition of the [Performance Web API]{@link https://developer.mozilla.org/en-US/docs/Web/API/Performance}
  * for accessing a high resolution monotonic clock.
@@ -89,21 +71,14 @@ function getNodePerformance(): Performance | undefined {
 }
 
 /**
- * The Performance API implementation for the current platform, if available.
+ * Returns a timestamp in seconds since the UNIX epoch using the Date API.
  */
-const platformPerformance: Performance | undefined = isNodeEnv() ? getNodePerformance() : getBrowserPerformance();
-
-const timestampSource: TimestampSource =
-  platformPerformance === undefined
-    ? dateTimestampSource
-    : {
-        nowSeconds: () => (platformPerformance.timeOrigin + platformPerformance.now()) / 1000,
-      };
+export const dateTimestampInSeconds: () => number = () => Date.now() / 1000;
 
 /**
- * Returns a timestamp in seconds since the UNIX epoch using the Date API.
+ * The Performance API implementation for the current platform, if available.
  */
-export const dateTimestampInSeconds: () => number = dateTimestampSource.nowSeconds.bind(dateTimestampSource);
+const platformPerformance: Performance | undefined = isNodeEnv() ? getNodePerformance() : getBrowserPerformance();
 
 /**
  * Returns a timestamp in seconds since the UNIX epoch using either the Performance or Date APIs, depending on the
@@ -116,7 +91,10 @@ export const dateTimestampInSeconds: () => number = dateTimestampSource.nowSecon
  * skew can grow to arbitrary amounts like days, weeks or months.
  * See https://github.com/getsentry/sentry-javascript/issues/2590.
  */
-export const timestampInSeconds: () => number = timestampSource.nowSeconds.bind(timestampSource);
+export const timestampInSeconds =
+  platformPerformance === undefined
+    ? dateTimestampInSeconds
+    : () => (platformPerformance.timeOrigin + platformPerformance.now()) / 1000;
 
 // Re-exported with an old name for backwards-compatibility.
 export const timestampWithMs = timestampInSeconds;

};

/**
Expand Down Expand Up @@ -97,7 +97,7 @@ const timestampSource: TimestampSource =
platformPerformance === undefined
? dateTimestampSource
: {
nowSeconds: () => (platformPerformance.timeOrigin + platformPerformance.now()) / 1000,
nowSeconds: () => msToSec(platformPerformance.timeOrigin + platformPerformance.now()),
};

/**
Expand Down Expand Up @@ -183,3 +183,19 @@ export const browserPerformanceTimeOrigin = ((): number | undefined => {
_browserPerformanceTimeOriginMode = 'dateNow';
return dateNow;
})();

/**
* Converts from milliseconds to seconds
* @param time time in ms
*/
export function msToSec(time: number): number {
return time / 1000;
}

/**
* Converts from seconds to milliseconds
* @param time time in seconds
*/
export function secToMs(time: number): number {
return time * 1000;
}