Skip to content

Commit

Permalink
ref(tracing): Idle transaction refactoring (#3988)
Browse files Browse the repository at this point in the history 
* simplify heartbeatString creation

* fix initTimeout return type + jsdoc for better discoverability in IDE

* remove unused reference heart beat timeout

* dedupe HEARTBEAT_INTERVAL between src and test

* replace 1000 with DEFAULT_IDLE_TIMEOUT
  • Loading branch information
@naseemkullah
naseemkullah authored Sep 20, 2021
1 parent 4ea7b5d commit eea6d54
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 29 deletions.
26 changes: 13 additions & 13 deletions packages/tracing/src/idletransaction.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { SpanStatus } from './spanstatus';
import { Transaction } from './transaction';

export const DEFAULT_IDLE_TIMEOUT = 1000;
export const HEARTBEAT_INTERVAL = 5000;

/**
* @inheritDoc
Expand Down Expand Up @@ -55,9 +56,6 @@ export class IdleTransaction extends Transaction {
// Activities store a list of active spans
public activities: Record<string, boolean> = {};

// Stores reference to the timeout that calls _beat().
private _heartbeatTimer: number = 0;

// Track state of activities in previous heartbeat
private _prevHeartbeatString: string | undefined;

Expand All @@ -69,15 +67,19 @@ export class IdleTransaction extends Transaction {

private readonly _beforeFinishCallbacks: BeforeFinishCallback[] = [];

// If a transaction is created and no activities are added, we want to make sure that
// it times out properly. This is cleared and not used when activities are added.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
private _initTimeout: any;
/**
* If a transaction is created and no activities are added, we want to make sure that
* it times out properly. This is cleared and not used when activities are added.
*/
private _initTimeout: ReturnType<typeof setTimeout> | undefined;

public constructor(
transactionContext: TransactionContext,
private readonly _idleHub?: Hub,
// The time to wait in ms until the idle transaction will be finished. Default: 1000
/**
* The time to wait in ms until the idle transaction will be finished.
* @default 1000
*/
private readonly _idleTimeout: number = DEFAULT_IDLE_TIMEOUT,
// If an idle transaction should be put itself on and off the scope automatically.
private readonly _onScope: boolean = false,
Expand Down Expand Up @@ -232,14 +234,12 @@ export class IdleTransaction extends Transaction {
* If this occurs we finish the transaction.
*/
private _beat(): void {
clearTimeout(this._heartbeatTimer);
// We should not be running heartbeat if the idle transaction is finished.
if (this._finished) {
return;
}

const keys = Object.keys(this.activities);
const heartbeatString = keys.length ? keys.reduce((prev: string, current: string) => prev + current) : '';
const heartbeatString = Object.keys(this.activities).join('');

if (heartbeatString === this._prevHeartbeatString) {
this._heartbeatCounter += 1;
Expand All @@ -264,9 +264,9 @@ export class IdleTransaction extends Transaction {
*/
private _pingHeartbeat(): void {
logger.log(`pinging Heartbeat -> current counter: ${this._heartbeatCounter}`);
this._heartbeatTimer = (setTimeout(() => {
setTimeout(() => {
this._beat();
}, 5000) as unknown) as number;
}, HEARTBEAT_INTERVAL);
}
}

Expand Down
36 changes: 20 additions & 16 deletions packages/tracing/test/idletransaction.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { BrowserClient } from '@sentry/browser';
import { Hub } from '@sentry/hub';

import { DEFAULT_IDLE_TIMEOUT, IdleTransaction, IdleTransactionSpanRecorder } from '../src/idletransaction';
import {
DEFAULT_IDLE_TIMEOUT,
HEARTBEAT_INTERVAL,
IdleTransaction,
IdleTransactionSpanRecorder,
} from '../src/idletransaction';
import { Span } from '../src/span';
import { SpanStatus } from '../src/spanstatus';

Expand All @@ -13,7 +18,7 @@ beforeEach(() => {
describe('IdleTransaction', () => {
describe('onScope', () => {
it('sets the transaction on the scope on creation if onScope is true', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000, true);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT, true);
transaction.initSpanRecorder(10);

hub.configureScope(s => {
Expand All @@ -22,7 +27,7 @@ describe('IdleTransaction', () => {
});

it('does not set the transaction on the scope on creation if onScope is falsey', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT);
transaction.initSpanRecorder(10);

hub.configureScope(s => {
Expand All @@ -31,7 +36,7 @@ describe('IdleTransaction', () => {
});

it('removes sampled transaction from scope on finish if onScope is true', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000, true);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT, true);
transaction.initSpanRecorder(10);

transaction.finish();
Expand All @@ -43,7 +48,7 @@ describe('IdleTransaction', () => {
});

it('removes unsampled transaction from scope on finish if onScope is true', () => {
const transaction = new IdleTransaction({ name: 'foo', sampled: false }, hub, 1000, true);
const transaction = new IdleTransaction({ name: 'foo', sampled: false }, hub, DEFAULT_IDLE_TIMEOUT, true);

transaction.finish();
jest.runAllTimers();
Expand All @@ -59,7 +64,7 @@ describe('IdleTransaction', () => {
});

it('push and pops activities', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT);
const mockFinish = jest.spyOn(transaction, 'finish');
transaction.initSpanRecorder(10);
expect(transaction.activities).toMatchObject({});
Expand All @@ -77,7 +82,7 @@ describe('IdleTransaction', () => {
});

it('does not push activities if a span already has an end timestamp', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT);
transaction.initSpanRecorder(10);
expect(transaction.activities).toMatchObject({});

Expand All @@ -86,7 +91,7 @@ describe('IdleTransaction', () => {
});

it('does not finish if there are still active activities', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT);
const mockFinish = jest.spyOn(transaction, 'finish');
transaction.initSpanRecorder(10);
expect(transaction.activities).toMatchObject({});
Expand All @@ -105,7 +110,7 @@ describe('IdleTransaction', () => {
it('calls beforeFinish callback before finishing', () => {
const mockCallback1 = jest.fn();
const mockCallback2 = jest.fn();
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT);
transaction.initSpanRecorder(10);
transaction.registerBeforeFinishCallback(mockCallback1);
transaction.registerBeforeFinishCallback(mockCallback2);
Expand All @@ -124,7 +129,7 @@ describe('IdleTransaction', () => {
});

it('filters spans on finish', () => {
const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234 }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234 }, hub, DEFAULT_IDLE_TIMEOUT);
transaction.initSpanRecorder(10);

// regular child - should be kept
Expand Down Expand Up @@ -158,15 +163,15 @@ describe('IdleTransaction', () => {

describe('_initTimeout', () => {
it('finishes if no activities are added to the transaction', () => {
const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234 }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234 }, hub, DEFAULT_IDLE_TIMEOUT);
transaction.initSpanRecorder(10);

jest.advanceTimersByTime(DEFAULT_IDLE_TIMEOUT);
expect(transaction.endTimestamp).toBeDefined();
});

it('does not finish if a activity is started', () => {
const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234 }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234 }, hub, DEFAULT_IDLE_TIMEOUT);
transaction.initSpanRecorder(10);
transaction.startChild({});

Expand All @@ -177,7 +182,6 @@ describe('IdleTransaction', () => {

describe('heartbeat', () => {
it('does not mark transaction as `DeadlineExceeded` if idle timeout has not been reached', () => {
const HEARTBEAT_INTERVAL = 5000;
// 20s to exceed 3 heartbeats
const transaction = new IdleTransaction({ name: 'foo' }, hub, 20000);
const mockFinish = jest.spyOn(transaction, 'finish');
Expand All @@ -202,7 +206,7 @@ describe('IdleTransaction', () => {
});

it('finishes a transaction after 3 beats', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT);
const mockFinish = jest.spyOn(transaction, 'finish');
transaction.initSpanRecorder(10);

Expand All @@ -223,7 +227,7 @@ describe('IdleTransaction', () => {
});

it('resets after new activities are added', () => {
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT);
const mockFinish = jest.spyOn(transaction, 'finish');
transaction.initSpanRecorder(10);

Expand Down Expand Up @@ -315,7 +319,7 @@ describe('IdleTransactionSpanRecorder', () => {
const mockPushActivity = jest.fn();
const mockPopActivity = jest.fn();

const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
const transaction = new IdleTransaction({ name: 'foo' }, hub, DEFAULT_IDLE_TIMEOUT);
const spanRecorder = new IdleTransactionSpanRecorder(mockPushActivity, mockPopActivity, transaction.spanId, 10);

spanRecorder.add(transaction);
Expand Down

0 comments on commit eea6d54

Please sign in to comment.