Skip to content

Commit b435b27

Browse files
AbhiPrasadAbhiPrasad
committed
refactor heartbeat
1 parent 858367d commit b435b27

File tree

4 files changed

+197
-33
lines changed

4 files changed

+197
-33
lines changed

‎packages/tracing/src/idletransaction.ts‎

Lines changed: 45 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,16 @@
11
// tslint:disable:max-classes-per-file
22
import { Hub } from '@sentry/hub';
33
import { TransactionContext } from '@sentry/types';
4-
import { timestampWithMs, logger } from '@sentry/utils';
4+
import { logger, timestampWithMs } from '@sentry/utils';
55

66
import { Span } from './span';
77
import { SpanStatus } from './spanstatus';
88
import { SpanRecorder, Transaction } from './transaction';
9-
import { BrowserTracing } from './integrations/browsertracing';
109

1110
/**
1211
* @inheritDoc
1312
*/
14-
class IdleTransactionSpanRecorder extends SpanRecorder {
13+
export class IdleTransactionSpanRecorder extends SpanRecorder {
1514
private readonly _pushActivity?: (id: string) => void;
1615
private readonly _popActivity?: (id: string) => void;
1716

@@ -25,8 +24,10 @@ class IdleTransactionSpanRecorder extends SpanRecorder {
2524
* @inheritDoc
2625
*/
2726
public add(span: Span): void {
27+
// tslint:disable-next-line: no-unbound-method
28+
const oldFinish: Function = span.finish;
2829
span.finish = (endTimestamp?: number) => {
29-
span.finish(endTimestamp);
30+
oldFinish(endTimestamp);
3031
if (this._popActivity) {
3132
this._popActivity(span.spanId);
3233
}
@@ -47,7 +48,7 @@ export class IdleTransaction extends Transaction {
4748
/**
4849
* Activities store a list of active spans
4950
*/
50-
public _activities: Record<string, boolean> = {};
51+
public activities: Record<string, boolean> = {};
5152

5253
private _heartbeatTimer: number = 0;
5354

@@ -64,32 +65,34 @@ export class IdleTransaction extends Transaction {
6465
}
6566

6667
/**
67-
* Checks when entries of this._activities are not changing for 3 beats.
68+
* Checks when entries of this.activities are not changing for 3 beats.
6869
* If this occurs we finish the transaction.
6970
*/
7071
private _beat(): void {
7172
clearTimeout(this._heartbeatTimer);
72-
const keys = Object.keys(this._activities);
73-
if (keys.length) {
74-
const heartbeatString = keys.reduce((prev: string, current: string) => prev + current);
75-
if (heartbeatString === this._prevHeartbeatString) {
76-
this._heartbeatCounter++;
77-
} else {
78-
this._heartbeatCounter = 0;
79-
}
80-
if (this._heartbeatCounter >= 3) {
81-
logger.log(
82-
`[Tracing] Transaction: ${
83-
SpanStatus.Cancelled
84-
} -> Heartbeat safeguard kicked in since content hasn't changed for 3 beats`,
85-
);
86-
this.setStatus(SpanStatus.DeadlineExceeded);
87-
this.setTag('heartbeat', 'failed');
88-
this._finishIdleTransaction(timestampWithMs());
89-
}
90-
this._prevHeartbeatString = heartbeatString;
73+
const keys = Object.keys(this.activities);
74+
const heartbeatString = keys.length ? keys.reduce((prev: string, current: string) => prev + current) : '';
75+
76+
if (heartbeatString === this._prevHeartbeatString) {
77+
this._heartbeatCounter++;
78+
} else {
79+
this._heartbeatCounter = 1;
80+
}
81+
82+
this._prevHeartbeatString = heartbeatString;
83+
84+
if (this._heartbeatCounter >= 3) {
85+
logger.log(
86+
`[Tracing] Transaction: ${
87+
SpanStatus.Cancelled
88+
} -> Heartbeat safeguard kicked in since content hasn't changed for 3 beats`,
89+
);
90+
this.setStatus(SpanStatus.DeadlineExceeded);
91+
this.setTag('heartbeat', 'failed');
92+
this._finishIdleTransaction(timestampWithMs());
93+
} else {
94+
this._pingHeartbeat();
9195
}
92-
this._pingHeartbeat();
9396
}
9497

9598
/**
@@ -131,6 +134,7 @@ export class IdleTransaction extends Transaction {
131134

132135
logger.log('[Tracing] flushing IdleTransaction');
133136
this.finish();
137+
this._finished = true;
134138
} else {
135139
logger.log('[Tracing] No active IdleTransaction');
136140
}
@@ -141,20 +145,20 @@ export class IdleTransaction extends Transaction {
141145
* @param spanId The span id that represents the activity
142146
*/
143147
private _pushActivity(spanId: string): void {
144-
this._activities[spanId] = true;
148+
this.activities[spanId] = true;
145149
}
146150

147151
/**
148152
* Remove an activity from usage
149153
* @param spanId The span id that represents the activity
150154
*/
151155
private _popActivity(spanId: string): void {
152-
if (this._activities[spanId]) {
156+
if (this.activities[spanId]) {
153157
// tslint:disable-next-line: no-dynamic-delete
154-
delete this._activities[spanId];
158+
delete this.activities[spanId];
155159
}
156160

157-
const count = Object.keys(this._activities).length;
161+
const count = Object.keys(this.activities).length;
158162
if (count === 0) {
159163
const timeout = this._idleTimeout;
160164
// We need to add the timeout here to have the real endtimestamp of the transaction
@@ -171,8 +175,18 @@ export class IdleTransaction extends Transaction {
171175
*/
172176
public initSpanRecorder(maxlen?: number): void {
173177
if (!this.spanRecorder) {
178+
const pushActivity = (id: string) => {
179+
if (id !== this.spanId) {
180+
this._pushActivity(id);
181+
}
182+
};
183+
const popActivity = (id: string) => {
184+
if (id !== this.spanId) {
185+
this._popActivity(id);
186+
}
187+
};
174188
// tslint:disable-next-line: no-unbound-method
175-
this.spanRecorder = new IdleTransactionSpanRecorder(maxlen, this._popActivity, this._pushActivity);
189+
this.spanRecorder = new IdleTransactionSpanRecorder(maxlen, pushActivity, popActivity);
176190
}
177191
this.spanRecorder.add(this);
178192
}

‎packages/tracing/src/integrations/tracing/router.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ import { addInstrumentationHandler, getGlobalObject, timestampWithMs } from '@se
33

44
import { Transaction } from '../../transaction';
55

6+
import { Location as LocationType } from './types';
7+
68
const global = getGlobalObject<Window>();
79

810
/**
@@ -42,7 +44,7 @@ export interface TracingRouterOptions {
4244
*
4345
* @param name the current name of the pageload/navigation transaction
4446
*/
45-
beforeNavigate(name: string): string | null;
47+
beforeNavigate(name: LocationType): string | null;
4648
}
4749

4850
/** JSDOC */
@@ -77,7 +79,7 @@ export class TracingRouter implements RoutingInstrumentation {
7779

7880
let name: string | null = window.location.pathname;
7981
if (this.options.beforeNavigate) {
80-
name = this.options.beforeNavigate(name);
82+
name = this.options.beforeNavigate(window.location);
8183

8284
// if beforeNavigate returns null, we should not start a transaction.
8385
if (name === null) {

‎packages/tracing/src/integrations/types.ts‎ renamed to ‎packages/tracing/src/integrations/tracing/types.ts‎

File renamed without changes.

‎packages/tracing/test/idletransaction.test.ts‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
import { BrowserClient } from '@sentry/browser';
2+
import { Hub, Scope } from '@sentry/hub';
3+
4+
import { IdleTransaction, IdleTransactionSpanRecorder } from '../src/idletransaction';
5+
import { Span } from '../src/span';
6+
7+
describe('IdleTransaction', () => {
8+
let hub: Hub;
9+
beforeEach(() => {
10+
jest.useFakeTimers();
11+
hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
12+
});
13+
14+
it('push and pops activities', () => {
15+
const mockFinish = jest.fn();
16+
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
17+
transaction.finish = mockFinish;
18+
transaction.initSpanRecorder(10);
19+
expect(transaction.activities).toMatchObject({});
20+
21+
const span = transaction.startChild();
22+
expect(transaction.activities).toMatchObject({ [span.spanId]: true });
23+
24+
expect(mockFinish).toHaveBeenCalledTimes(0);
25+
26+
span.finish();
27+
expect(transaction.activities).toMatchObject({});
28+
jest.runOnlyPendingTimers();
29+
30+
expect(mockFinish).toHaveBeenCalledTimes(1);
31+
});
32+
33+
it('does not finish if there are still active activities', () => {
34+
const mockFinish = jest.fn();
35+
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
36+
37+
transaction.finish = mockFinish;
38+
transaction.initSpanRecorder(10);
39+
expect(transaction.activities).toMatchObject({});
40+
41+
const span = transaction.startChild();
42+
const childSpan = span.startChild();
43+
44+
expect(transaction.activities).toMatchObject({ [span.spanId]: true, [childSpan.spanId]: true });
45+
span.finish();
46+
jest.runOnlyPendingTimers();
47+
48+
expect(mockFinish).toHaveBeenCalledTimes(0);
49+
expect(transaction.activities).toMatchObject({ [childSpan.spanId]: true });
50+
});
51+
52+
describe('heartbeat', () => {
53+
it('finishes a transaction after 3 beats', () => {
54+
const mockFinish = jest.fn();
55+
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
56+
transaction.finish = mockFinish;
57+
transaction.initSpanRecorder(10);
58+
59+
expect(mockFinish).toHaveBeenCalledTimes(0);
60+
61+
// Beat 1
62+
jest.runOnlyPendingTimers();
63+
expect(mockFinish).toHaveBeenCalledTimes(0);
64+
65+
// Beat 2
66+
jest.runOnlyPendingTimers();
67+
expect(mockFinish).toHaveBeenCalledTimes(0);
68+
69+
// Beat 3
70+
jest.runOnlyPendingTimers();
71+
expect(mockFinish).toHaveBeenCalledTimes(1);
72+
});
73+
74+
it('resets after new activities are added', () => {
75+
const mockFinish = jest.fn();
76+
const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
77+
transaction.finish = mockFinish;
78+
transaction.initSpanRecorder(10);
79+
80+
expect(mockFinish).toHaveBeenCalledTimes(0);
81+
82+
// Beat 1
83+
jest.runOnlyPendingTimers();
84+
expect(mockFinish).toHaveBeenCalledTimes(0);
85+
86+
const span = transaction.startChild(); // push activity
87+
88+
// Beat 1
89+
jest.runOnlyPendingTimers();
90+
expect(mockFinish).toHaveBeenCalledTimes(0);
91+
92+
// Beat 2
93+
jest.runOnlyPendingTimers();
94+
expect(mockFinish).toHaveBeenCalledTimes(0);
95+
96+
transaction.startChild(); // push activity
97+
transaction.startChild(); // push activity
98+
99+
// Beat 1
100+
jest.runOnlyPendingTimers();
101+
expect(mockFinish).toHaveBeenCalledTimes(0);
102+
103+
// Beat 2
104+
jest.runOnlyPendingTimers();
105+
expect(mockFinish).toHaveBeenCalledTimes(0);
106+
107+
span.finish(); // pop activity
108+
109+
// Beat 1
110+
jest.runOnlyPendingTimers();
111+
expect(mockFinish).toHaveBeenCalledTimes(0);
112+
113+
// Beat 2
114+
jest.runOnlyPendingTimers();
115+
expect(mockFinish).toHaveBeenCalledTimes(0);
116+
117+
// Beat 3
118+
jest.runOnlyPendingTimers();
119+
expect(mockFinish).toHaveBeenCalledTimes(1);
120+
});
121+
});
122+
});
123+
124+
describe('IdleTransactionSpanRecorder', () => {
125+
it('pushes and pops activities', () => {
126+
const mockPushActivity = jest.fn();
127+
const mockPopActivity = jest.fn();
128+
129+
const spanRecorder = new IdleTransactionSpanRecorder(10, mockPushActivity, mockPopActivity);
130+
expect(mockPushActivity).toHaveBeenCalledTimes(0);
131+
expect(mockPopActivity).toHaveBeenCalledTimes(0);
132+
133+
const span = new Span({ sampled: true });
134+
135+
expect(spanRecorder.spans).toHaveLength(0);
136+
spanRecorder.add(span);
137+
expect(spanRecorder.spans).toHaveLength(1);
138+
139+
expect(mockPushActivity).toHaveBeenCalledTimes(1);
140+
expect(mockPushActivity).toHaveBeenLastCalledWith(span.spanId);
141+
expect(mockPopActivity).toHaveBeenCalledTimes(0);
142+
143+
span.finish();
144+
expect(mockPushActivity).toHaveBeenCalledTimes(1);
145+
expect(mockPopActivity).toHaveBeenCalledTimes(1);
146+
expect(mockPushActivity).toHaveBeenLastCalledWith(span.spanId);
147+
});
148+
});

0 commit comments

Comments
 (0)