8000 feat(core): Deprecate span `startTimestamp` & `endTimestamp` (#10192) · GingerAdonis/sentry-javascript@2974df6 · GitHub
[go: up one dir, main page]
Skip to content

Commit 2974df6

Browse files
mydeamydea
authored
feat(core): Deprecate span startTimestamp & endTimestamp (getsentry#10192)
Instead, you can use `spanToJSON()` to _read_ the timestamps. Updating timestamps after span creation will not be possible in v8. Also ensure that `span.end()` can only be called once - we already do that for transactions, and it saves us from checking e.g. `span.endTimestamp` before calling `span.end()`. This is also aligned with how OTEL does it (they emit a warning if trying to end a span twice).
1 parent 569d782 commit 2974df6

File tree

24 files changed

+206
-134
lines changed

24 files changed

+206
-134
lines changed

‎MIGRATION.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,9 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
183183
- `transaction.setMeasurement()`: Use `Sentry.setMeasurement()` instead. In v8, setting measurements will be limited to
184184
the currently active root span.
185185
- `transaction.setName()`: Set the name with `.updateName()` and the source with `.setAttribute()` instead.
186+
- `span.startTimestamp`: use `spanToJSON(span).start_timestamp` instead. You cannot update this anymore in v8.
187+
- `span.endTimestamp`: use `spanToJSON(span).timestamp` instead. You cannot update this anymore in v8. You can pass a
188+
custom end timestamp to `span.end(endTimestamp)`.
186189

187190
## Deprecate `pushScope` & `popScope` in favor of `withScope`
188191

‎packages/core/src/tracing/idletransaction.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { logger, timestampInSeconds } from '@sentry/utils';
44

55
import { DEBUG_BUILD } from '../debug-build';
66
import type { Hub } from '../hub';
7-
import { spanTimeInputToSeconds } from '../utils/spanUtils';
7+
import { spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils';
88
import type { Span } from './span';
99
import { SpanRecorder } from './span';
1010
import { Transaction } from './transaction';
@@ -55,7 +55,7 @@ export class IdleTransactionSpanRecorder extends SpanRecorder {
5555
};
5656

5757
// We should only push new activities if the span does not have an end timestamp.
58-
if (span.endTimestamp === undefined) {
58+
if (spanToJSON(span).timestamp === undefined) {
5959
this._pushActivity(span.spanContext().spanId);
6060
}
6161
}
@@ -167,18 +167,19 @@ export class IdleTransaction extends Transaction {
167167
}
168168

169169
// We cancel all pending spans with status "cancelled" to indicate the idle transaction was finished early
170-
if (!span.endTimestamp) {
171-
span.endTimestamp = endTimestampInS;
170+
if (!spanToJSON(span).timestamp) {
172171
span.setStatus('cancelled');
172+
span.end(endTimestampInS);
173173
DEBUG_BUILD &&
174174
logger.log('[Tracing] cancelling span since transaction ended early', JSON.stringify(span, undefined, 2));
175175
}
176176

177-
const spanStartedBeforeTransactionFinish = span.startTimestamp < endTimestampInS;
177+
const { start_timestamp: startTime, timestamp: endTime } = spanToJSON(span);
178+
const spanStartedBeforeTransactionFinish = startTime && startTime < endTimestampInS;
178179

179180
// Add a delta with idle timeout so that we prevent false positives
180181
const timeoutWithMarginOfError = (this._finalTimeout + this._idleTimeout) / 1000;
181-
const spanEndedBeforeFinalTimeout = span.endTimestamp - this.startTimestamp < timeoutWithMarginOfError;
182+
const spanEndedBeforeFinalTimeout = endTime && startTime && endTime - startTime < timeoutWithMarginOfError;
182183

183184
if (DEBUG_BUILD) {
184185
const stringifiedSpan = JSON.stringify(span, undefined, 2);

‎packages/core/src/tracing/span.ts

Lines changed: 50 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -71,16 +71,6 @@ export class Span implements SpanInterface {
7171
*/
7272
public status?: SpanStatusType | string;
7373

74-
/**
75-
* Timestamp in seconds when the span was created.
76-
*/
77-
public startTimestamp: number;
78-
79-
/**
80-
* Timestamp in seconds when the span ended.
81-
*/
82-
public endTimestamp?: number;
83-
8474
/**
8575
* @inheritDoc
8676
*/
@@ -131,6 +121,10 @@ export class Span implements SpanInterface {
131121
protected _sampled: boolean | undefined;
132122
protected _name?: string;
133123
protected _attributes: SpanAttributes;
124+
/** Epoch timestamp in seconds when the span started. */
125+
protected _startTime: number;
126+
/** Epoch timestamp in seconds when the span ended. */
127+
protected _endTime?: number;
134128

135129
private _logMessage?: string;
136130

@@ -144,7 +138,7 @@ export class Span implements SpanInterface {
144138
public constructor(spanContext: SpanContext = {}) {
145139
this._traceId = spanContext.traceId || uuid4();
146140
this._spanId = spanContext.spanId || uuid4().substring(16);
147-
this.startTimestamp = spanContext.startTimestamp || timestampInSeconds();
141+
this._startTime = spanContext.startTimestamp || timestampInSeconds();
148142
// eslint-disable-next-line deprecation/deprecation
149143
this.tags = spanContext.tags ? { ...spanContext.tags } : {};
150144
// eslint-disable-next-line deprecation/deprecation
@@ -170,7 +164,7 @@ export class Span implements SpanInterface {
170164
this.status = spanContext.status;
171165
}
172166
if (spanContext.endTimestamp) {
173-
this.endTimestamp = spanContext.endTimestamp;
167+
this._endTime = spanContext.endTimestamp;
174168
}
175169
}
176170

@@ -273,6 +267,38 @@ export class Span implements SpanInterface {
273267
this._attributes = attributes;
274268
}
275269

270+
/**
271+
* Timestamp in seconds (epoch time) indicating when the span started.
272+
* @deprecated Use `spanToJSON()` instead.
273+
*/
274+
public get startTimestamp(): number {
275+
return this._startTime;
276+
}
277+
278+
/**
279+
* Timestamp in seconds (epoch time) indicating when the span started.
280+
* @deprecated In v8, you will not be able to update the span start time after creation.
281+
*/
282+
public set startTimestamp(startTime: number) {
283+
this._startTime = startTime;
284+
}
285+
286+
/**
287+
* Timestamp in seconds when the span ended.
288+
* @deprecated Use `spanToJSON()` instead.
289+
*/
290+
public get endTimestamp(): number | undefined {
291+
return this._endTime;
292+
}
293+
294+
/**
295+
* Timestamp in seconds when the span ended.
296+
* @deprecated Set the end time via `span.end()` instead.
297+
*/
298+
public set endTimestamp(endTime: number | undefined) {
299+
this._endTime = endTime;
300+
}
301+
276302
/* eslint-enable @typescript-eslint/member-ordering */
277303

278304
/** @inheritdoc */
@@ -426,6 +452,10 @@ export class Span implements SpanInterface {
426452

427453
/** @inheritdoc */
428454
public end(endTimestamp?: SpanTimeInput): void {
455+
// If already ended, skip
456+
if (this._endTime) {
457+
return;
458+
}
429459
const rootSpan = getRootSpan(this);
430460
if (
431461
DEBUG_BUILD &&
@@ -439,7 +469,7 @@ export class Span implements SpanInterface {
439469
}
440470
}
441471

442-
this.endTimestamp = spanTimeInputToSeconds(endTimestamp);
472+
this._endTime = spanTimeInputToSeconds(endTimestamp);
443473
}
444474

445475
/**
@@ -460,12 +490,12 @@ export class Span implements SpanInterface {
460490
return dropUndefinedKeys({
461491
data: this._getData(),
462492
description: this._name,
463-
endTimestamp: this.endTimestamp,
493+
endTimestamp: this._endTime,
464494
op: this.op,
465495
parentSpanId: this.parentSpanId,
466496
sampled: this._sampled,
467497
spanId: this._spanId,
468-
startTimestamp: this.startTimestamp,
498+
startTimestamp: this._startTime,
469499
status: this.status,
470500
// eslint-disable-next-line deprecation/deprecation
471501
tags: this.tags,
@@ -483,12 +513,12 @@ export class Span implements SpanInterface {
483513
this.data = spanContext.data || {};
484514
// eslint-disable-next-line deprecation/deprecation
485515
this._name = spanContext.name || spanContext.description;
486-
this.endTimestamp = spanContext.endTimestamp;
516+
this._endTime = spanContext.endTimestamp;
487517
this.op = spanContext.op;
488518
this.parentSpanId = spanContext.parentSpanId;
489519
this._sampled = spanContext.sampled;
490520
this._spanId = spanContext.spanId || this._spanId;
491-
this.startTimestamp = spanContext.startTimestamp || this.startTimestamp;
521+
this._startTime = spanContext.startTimestamp || this._startTime;
492522
this.status = spanContext.status;
493523
// eslint-disable-next-line deprecation/deprecation
494524
this.tags = spanContext.tags || {};
@@ -521,19 +551,19 @@ export class Span implements SpanInterface {
521551
op: this.op,
522552
parent_span_id: this.parentSpanId,
523553
span_id: this._spanId,
524-
start_timestamp: this.startTimestamp,
554+
start_timestamp: this._startTime,
525555
status: this.status,
526556
// eslint-disable-next-line deprecation/deprecation
527557
tags: Object.keys(this.tags).length > 0 ? this.tags : undefined,
528-
timestamp: this.endTimestamp,
558+
timestamp: this._endTime,
529559
trace_id: this._traceId,
530560
origin: this.origin,
531561
});
532562
}
533563

534564
/** @inheritdoc */
535565
public isRecording(): boolean {
536-
return !this.endTimestamp && !!this._sampled;
566+
return !this._endTime && !!this._sampled;
537567
}
538568

539569
/**

‎packages/core/src/tracing/trace.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ export function startSpanManual<T>(
242242
() => callback(activeSpan, finishAndSetSpan),
243243
() => {
244244
// Only update the span status if it hasn't been changed yet, and the span is not yet finished
245-
if (activeSpan && !activeSpan.endTimestamp && (!activeSpan.status || activeSpan.status === 'ok')) {
245+
if (activeSpan && activeSpan.isRecording() && (!activeSpan.status || activeSpan.status === 'ok')) {
246246
activeSpan.setStatus('internal_error');
247247
}
248248
},

‎packages/core/src/tracing/transaction.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import { DEBUG_BUILD } from '../debug-build';
1616
import type { Hub } from '../hub';
1717
import { getCurrentHub } from '../hub';
1818
import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes';
19-
import { spanTimeInputToSeconds, spanToTraceContext } from '../utils/spanUtils';
19+
import { spanTimeInputToSeconds, spanToJSON, spanToTraceContext } from '../utils/spanUtils';
2020
import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
2121
import { Span as SpanClass, SpanRecorder } from './span';
2222

@@ -257,7 +257,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
257257
*/
258258
protected _finishTransaction(endTimestamp?: number): TransactionEvent | undefined {
259259
// This transaction is already finished, so we should not flush it again.
260-
if (this.endTimestamp !== undefined) {
260+
if (this._endTime !== undefined) {
261261
return undefined;
262262
}
263263

@@ -286,15 +286,15 @@ export class Transaction extends SpanClass implements TransactionInterface {
286286
return undefined;
287287
}
288288

289-
const finishedSpans = this.spanRecorder ? this.spanRecorder.spans.filter(s => s !== this && s.endTimestamp) : [];
289+
const finishedSpans = this.spanRecorder
290+
? this.spanRecorder.spans.filter(span => span !== this && spanToJSON(span).timestamp)
291+
: [];
290292

291293
if (this._trimEnd && finishedSpans.length > 0) {
292-
this.endTimestamp = finishedSpans.reduce((prev: SpanClass, current: SpanClass) => {
293-
if (prev.endTimestamp && current.endTimestamp) {
294-
return prev.endTimestamp > current.endTimestamp ? prev : current;
295-
}
296-
return prev;
297-
}).endTimestamp;
294+
const endTimes = finishedSpans.map(span => spanToJSON(span).timestamp).filter(Boolean) as number[];
295+
this._endTime = endTimes.reduce((prev, current) => {
296+
return prev > current ? prev : current;
297+
});
298298
}
299299

< F438 /td>
300300
// eslint-disable-next-line deprecation/deprecation
@@ -310,10 +310,10 @@ export class Transaction extends SpanClass implements TransactionInterface {
310310
},
311311
// TODO: Pass spans serialized via `spanToJSON()` here instead in v8.
312312
spans: finishedSpans,
313-
start_timestamp: this.startTimestamp,
313+
start_timestamp: this._startTime,
314314
// eslint-disable-next-line deprecation/deprecation
315315
tags: this.tags,
316-
timestamp: this.endTimestamp,
316+
timestamp: this._endTime,
317317
transaction: this._name,
318318
type: 'transaction',
319319
sdkProcessingMetadata: {

‎packages/core/test/lib/tracing/span.test.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { timestampInSeconds } from '@sentry/utils';
22
import { Span } from '../../../src';
3-
import { TRACE_FLAG_NONE, TRACE_FLAG_SAMPLED } from '../../../src/utils/spanUtils';
3+
import { TRACE_FLAG_NONE, TRACE_FLAG_SAMPLED, spanToJSON } from '../../../src/utils/spanUtils';
44

55
describe('span', () => {
66
describe('name', () => {
@@ -186,31 +186,41 @@ describe('span', () => {
186186
const now = timestampInSeconds();
187187
span.end();
188188

189-
expect(span.endTimestamp).toBeGreaterThanOrEqual(now);
189+
expect(spanToJSON(span).timestamp).toBeGreaterThanOrEqual(now);
190190
});
191191

192192
it('works with endTimestamp in seconds', () => {
193193
const span = new Span();
194194
const timestamp = timestampInSeconds() - 1;
195195
span.end(timestamp);
196196

197-
expect(span.endTimestamp).toEqual(timestamp);
197+
expect(spanToJSON(span).timestamp).toEqual(timestamp);
198198
});
199199

200200
it('works with endTimestamp in milliseconds', () => {
201201
const span = new Span();
202202
const timestamp = Date.now() - 1000;
203203
span.end(timestamp);
204204

205-
expect(span.endTimestamp).toEqual(timestamp / 1000);
205+
expect(spanToJSON(span).timestamp).toEqual(timestamp / 1000);
206206
});
207207

208208
it('works with endTimestamp in array form', () => {
209209
const span = new Span();
210210
const seconds = Math.floor(timestampInSeconds() - 1);
211211
span.end([seconds, 0]);
212212

213-
expect(span.endTimestamp).toEqual(seconds);
213+
expect(spanToJSON(span).timestamp).toEqual(seconds);
214+
});
215+
216+
it('skips if span is already ended', () => {
217+
const startTimestamp = timestampInSeconds() - 5;
218+
const endTimestamp = timestampInSeconds() - 1;
219+
const span = new Span({ startTimestamp, endTimestamp });
220+
221+
span.end();
222+
223+
expect(spanToJSON(span).timestamp).toBe(endTimestamp);
214224
});
215225
});
216226

‎packages/core/test/lib/tracing/trace.test.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Hub, addTracingExtensions, getCurrentScope, makeMain } from '../../../src';
1+
import { Hub, addTracingExtensions, getCurrentScope, makeMain, spanToJSON } from '../../../src';
22
import { Scope } from '../../../src/scope';
33
import {
44
Span,
@@ -183,17 +183,17 @@ describe('startSpan', () => {
183183
let _span: Span | undefined;
184184
startSpan({ name: 'GET users/[id]' }, span => {
185185
expect(span).toBeDefined();
186-
expect(span?.endTimestamp).toBeUndefined();
186+
expect(spanToJSON(span!).timestamp).toBeUndefined();
187187
_span = span as Span;
188188
});
189189

190190
expect(_span).toBeDefined();
191-
expect(_span?.endTimestamp).toBeDefined();
191+
expect(spanToJSON(_span!).timestamp).toBeDefined();
192192
});
193193

194194
it('allows to pass a `startTime`', () => {
195195
const start = startSpan({ name: 'outer', startTime: [1234, 0] }, span => {
196-
return span?.startTimestamp;
196+
return spanToJSON(span!).start_timestamp;
197197
});
198198

199199
expect(start).toEqual(1234);
@@ -236,9 +236,9 @@ describe('startSpanManual', () => {
236236
it('creates & finishes span', async () => {
237237
startSpanManual({ name: 'GET users/[id]' }, (span, finish) => {
238238
expect(span).toBeDefined();
239-
expect(span?.endTimestamp).toBeUndefined();
239+
expect(spanToJSON(span!).timestamp).toBeUndefined();
240240
finish();
241-
expect(span?.endTimestamp).toBeDefined();
241+
expect(spanToJSON(span!).timestamp).toBeDefined();
242242
});
243243
});
244244

@@ -286,7 +286,7 @@ describe('startSpanManual', () => {
286286
it('allows to pass a `startTime`', () => {
287287
const start = startSpanManual({ name: 'outer', startTime: [1234, 0] }, span => {
288288
span?.end();
289-
return span?.startTimestamp;
289+
return spanToJSON(span!).start_timestamp;
290290
});
291291

292292
expect(start).toEqual(1234);
@@ -298,11 +298,11 @@ describe('startInactiveSpan', () => {
298298
const span = startInactiveSpan({ name: 'GET users/[id]' });
299299

300300
expect(span).toBeDefined();
301-
expect(span?.endTimestamp).toBeUndefined();
301+
expect(spanToJSON(span!).timestamp).toBeUndefined();
302302

303303
span?.end();
304304

305-
expect(span?.endTimestamp).toBeDefined();
305+
expect(spanToJSON(span!).timestamp).toBeDefined();
306306
});
307307

308308
it('does not set span on scope', () => {
@@ -335,7 +335,7 @@ describe('startInactiveSpan', () => {
335335

336336
it('allows to pass a `startTime`', () => {
337337
const span = startInactiveSpan({ name: 'outer', startTime: [1234, 0] });
338-
expect(span?.startTimestamp).toEqual(1234);
338+
expect(spanToJSON(span!).start_timestamp).toEqual(1234);
339339
});
340340
});
341341

0 commit comments

Comments
 (0)
0