From 691b1d2788be4797f6aacf68cf6b3f00cc35eab6 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 26 Jul 2023 11:17:09 -0400 Subject: [PATCH 1/4] fix(tracing): Trim idle transaction spans if they exceed final timeout --- packages/core/src/tracing/idletransaction.ts | 20 +++++++++++-------- packages/tracing/test/idletransaction.test.ts | 13 ++++++++++++ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/packages/core/src/tracing/idletransaction.ts b/packages/core/src/tracing/idletransaction.ts index 9f7964a33f3f..33cc3d79633d 100644 --- a/packages/core/src/tracing/idletransaction.ts +++ b/packages/core/src/tracing/idletransaction.ts @@ -158,15 +158,19 @@ export class IdleTransaction extends Transaction { logger.log('[Tracing] cancelling span since transaction ended early', JSON.stringify(span, undefined, 2)); } - const keepSpan = span.startTimestamp < endTimestamp; - if (!keepSpan) { - __DEBUG_BUILD__ && - logger.log( - '[Tracing] discarding Span since it happened after Transaction was finished', - JSON.stringify(span, undefined, 2), - ); + const spanStartedBeforeTransactionFinish = span.startTimestamp < endTimestamp; + const spanEndedBeforeFinalTimeout = span.endTimestamp < (this._finalTimeout + this._idleTimeout) / 1000; + + if (__DEBUG_BUILD__) { + const stringifiedSpan = JSON.stringify(span, undefined, 2); + if (!spanStartedBeforeTransactionFinish) { + logger.log('[Tracing] discarding Span since it happened after Transaction was finished', stringifiedSpan); + } else if (!spanEndedBeforeFinalTimeout) { + logger.log('[Tracing] discarding Span since it finished after Transaction final timeout', stringifiedSpan); + } } - return keepSpan; + + return spanStartedBeforeTransactionFinish && spanEndedBeforeFinalTimeout; }); __DEBUG_BUILD__ && logger.log('[Tracing] flushing IdleTransaction'); diff --git a/packages/tracing/test/idletransaction.test.ts b/packages/tracing/test/idletransaction.test.ts index 027559c02296..801050d84362 100644 --- a/packages/tracing/test/idletransaction.test.ts +++ b/packages/tracing/test/idletransaction.test.ts @@ -202,6 +202,19 @@ describe('IdleTransaction', () => { } }); + it('filters out spans that exceed final timeout', () => { + const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234 }, hub, 1000, 3000); + transaction.initSpanRecorder(10); + + const span = transaction.startChild({ startTimestamp: transaction.startTimestamp + 2 }); + span.finish(span.startTimestamp + 10 + 30 + 1); + + transaction.finish(transaction.startTimestamp + 10); + + expect(transaction.spanRecorder).toBeDefined(); + expect(transaction.spanRecorder!.spans).toHaveLength(1); + }); + it('should record dropped transactions', async () => { const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234, sampled: false }, hub, 1000); From 10f1ab90ee24703f34005805d1d142c3ac19c6bc Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 26 Jul 2023 17:27:22 -0400 Subject: [PATCH 2/4] make logic more clear --- packages/core/src/tracing/idletransaction.ts | 7 ++++++- packages/tracing/test/idletransaction.test.ts | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tracing/idletransaction.ts b/packages/core/src/tracing/idletransaction.ts index 33cc3d79633d..7becf221d306 100644 --- a/packages/core/src/tracing/idletransaction.ts +++ b/packages/core/src/tracing/idletransaction.ts @@ -159,7 +159,12 @@ export class IdleTransaction extends Transaction { } const spanStartedBeforeTransactionFinish = span.startTimestamp < endTimestamp; - const spanEndedBeforeFinalTimeout = span.endTimestamp < (this._finalTimeout + this._idleTimeout) / 1000; + + const timeoutWithDelta = (this._finalTimeout + this._idleTimeout) / 1000; + + const transactionDidNotExceedTimeout = endTimestamp - this.startTimestamp < timeoutWithDelta; + const spanDidNotExceedTimeout = span.endTimestamp - span.startTimestamp < timeoutWithDelta; + const spanEndedBeforeFinalTimeout = transactionDidNotExceedTimeout && spanDidNotExceedTimeout; if (__DEBUG_BUILD__) { const stringifiedSpan = JSON.stringify(span, undefined, 2); diff --git a/packages/tracing/test/idletransaction.test.ts b/packages/tracing/test/idletransaction.test.ts index 801050d84362..88a2fb4f6d1d 100644 --- a/packages/tracing/test/idletransaction.test.ts +++ b/packages/tracing/test/idletransaction.test.ts @@ -209,7 +209,7 @@ describe('IdleTransaction', () => { const span = transaction.startChild({ startTimestamp: transaction.startTimestamp + 2 }); span.finish(span.startTimestamp + 10 + 30 + 1); - transaction.finish(transaction.startTimestamp + 10); + transaction.finish(transaction.startTimestamp + 50); expect(transaction.spanRecorder).toBeDefined(); expect(transaction.spanRecorder!.spans).toHaveLength(1); From b44cdd902ca3f36c8ea5217b889469f9007eb745 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 27 Jul 2023 08:52:52 -0400 Subject: [PATCH 3/4] simplify boolean logic --- packages/core/src/tracing/idletransaction.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/core/src/tracing/idletransaction.ts b/packages/core/src/tracing/idletransaction.ts index 7becf221d306..a125dd718910 100644 --- a/packages/core/src/tracing/idletransaction.ts +++ b/packages/core/src/tracing/idletransaction.ts @@ -160,11 +160,9 @@ export class IdleTransaction extends Transaction { const spanStartedBeforeTransactionFinish = span.startTimestamp < endTimestamp; + // Add a delta with idle timeout so that we prevent false positives const timeoutWithDelta = (this._finalTimeout + this._idleTimeout) / 1000; - - const transactionDidNotExceedTimeout = endTimestamp - this.startTimestamp < timeoutWithDelta; - const spanDidNotExceedTimeout = span.endTimestamp - span.startTimestamp < timeoutWithDelta; - const spanEndedBeforeFinalTimeout = transactionDidNotExceedTimeout && spanDidNotExceedTimeout; + const spanEndedBeforeFinalTimeout = span.endTimestamp - this.startTimestamp < timeoutWithDelta; if (__DEBUG_BUILD__) { const stringifiedSpan = JSON.stringify(span, undefined, 2); From 6e3a02ff2721ae053f4bfeec16f9e2cc144c970e Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 27 Jul 2023 10:20:07 -0400 Subject: [PATCH 4/4] naming change --- packages/core/src/tracing/idletransaction.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tracing/idletransaction.ts b/packages/core/src/tracing/idletransaction.ts index a125dd718910..6b72113097fc 100644 --- a/packages/core/src/tracing/idletransaction.ts +++ b/packages/core/src/tracing/idletransaction.ts @@ -161,8 +161,8 @@ export class IdleTransaction extends Transaction { const spanStartedBeforeTransactionFinish = span.startTimestamp < endTimestamp; // Add a delta with idle timeout so that we prevent false positives - const timeoutWithDelta = (this._finalTimeout + this._idleTimeout) / 1000; - const spanEndedBeforeFinalTimeout = span.endTimestamp - this.startTimestamp < timeoutWithDelta; + const timeoutWithMarginOfError = (this._finalTimeout + this._idleTimeout) / 1000; + const spanEndedBeforeFinalTimeout = span.endTimestamp - this.startTimestamp < timeoutWithMarginOfError; if (__DEBUG_BUILD__) { const stringifiedSpan = JSON.stringify(span, undefined, 2);