Skip to content
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

Jump to bottom

Idle transaction refactoring #3988

Merged
merged 5 commits into from Sep 20, 2021
Merged

Idle transaction refactoring #3988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
43496a8
simplify heartbeatString creation
naseemkullah Sep 18, 2021
66c47fc
fix initTimeout return type + jsdoc for better discoverability in IDE
naseemkullah Sep 18, 2021
f1eb74d
remove unused reference heart beat timeout
naseemkullah Sep 18, 2021
efff4c0
dedupe HEARTBEAT_INTERVAL between src and test
naseemkullah Sep 18, 2021
5885781
replace 1000 with DEFAULT_IDLE_TIMEOUT
naseemkullah Sep 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 13 additions & 13 deletions packages/tracing/src/idletransaction.ts
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

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

Now I’m wondering why we had this clear timeout logic in the first place 🤔

// 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
Open in desktop
@@ -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