From f45595c88969eaf453b546d2970dfc1200afd7b2 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 13 Aug 2020 10:55:39 -0400 Subject: [PATCH 1/6] feat(tracing): Change resource span op name and add tags --- packages/eslint-config-sdk/src/index.js | 4 ++++ packages/tracing/src/browser/metrics.ts | 19 ++++++++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/eslint-config-sdk/src/index.js b/packages/eslint-config-sdk/src/index.js index 2b67173f403d..b4e7de5f211f 100644 --- a/packages/eslint-config-sdk/src/index.js +++ b/packages/eslint-config-sdk/src/index.js @@ -39,6 +39,10 @@ module.exports = { // Enforce type annotations to maintain consistency. This is especially important as // we have a public API, so we want changes to be very explicit. '@typescript-eslint/typedef': ['error', { arrowParameter: false }], + + // Although for most codebases inferencing the return type is fine, we explicitly ask to annotate + // all functions with a return type. This is so that intent is as clear as possible. We are guarding against + // cases where you accidently refactor a function's return type to be the wrong type. '@typescript-eslint/explicit-function-return-type': ['error', { allowExpressions: true }], // Consistent ordering of fields, methods and constructors for classes should be enforced diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 96ff9ed0b9e3..171fda38eadb 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines */ /* eslint-disable @typescript-eslint/no-explicit-any */ import { SpanContext } from '@sentry/types'; import { getGlobalObject, logger } from '@sentry/utils'; @@ -214,7 +215,7 @@ function addMeasureSpans( /** Create resource related spans */ function addResourceSpans( transaction: Transaction, - entry: Record, + entry: Record, resourceName: string, startTime: number, duration: number, @@ -226,14 +227,26 @@ function addResourceSpans( return undefined; } + const tags: Record = {}; + if (entry.transferSize) { + tags.transferSize = (entry.transferSize as number).toString(); + } + if (entry.encodedBodySize) { + tags.encodedBodySize = (entry.encodedBodySize as number).toString(); + } + if (entry.decodedBodySize) { + tags.decodedBodySize = (entry.decodedBodySize as number).toString(); + } + const startTimestamp = timeOrigin + startTime; const endTimestamp = startTimestamp + duration; _startChild(transaction, { - description: `${entry.initiatorType} ${resourceName}`, + description: resourceName, endTimestamp, - op: 'resource', + op: entry.initiatorType && entry.initiatorType !== '' ? `resource.${entry.initiatorType}` : 'resource', startTimestamp, + tags, }); return endTimestamp; From 943e2971d6b1196e7ff2cc1e2a01e73399b2d41d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 13 Aug 2020 14:39:19 -0400 Subject: [PATCH 2/6] test: resource spans --- packages/tracing/src/browser/metrics.ts | 21 ++-- packages/tracing/test/browser/metrics.test.ts | 107 +++++++++++++++++- 2 files changed, 120 insertions(+), 8 deletions(-) diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 171fda38eadb..e7de8efde927 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -212,10 +212,17 @@ function addMeasureSpans( return measureStartTimestamp; } +export interface ResourceEntry extends Record { + initiatorType?: string; + transferSize?: number; + encodedBodySize?: number; + decodedBodySize?: number; +} + /** Create resource related spans */ -function addResourceSpans( +export function addResourceSpans( transaction: Transaction, - entry: Record, + entry: ResourceEntry, resourceName: string, startTime: number, duration: number, @@ -227,15 +234,15 @@ function addResourceSpans( return undefined; } - const tags: Record = {}; + const data: Record = {}; if (entry.transferSize) { - tags.transferSize = (entry.transferSize as number).toString(); + data.transferSize = entry.transferSize; } if (entry.encodedBodySize) { - tags.encodedBodySize = (entry.encodedBodySize as number).toString(); + data.encodedBodySize = entry.encodedBodySize; } if (entry.decodedBodySize) { - tags.decodedBodySize = (entry.decodedBodySize as number).toString(); + data.decodedBodySize = entry.decodedBodySize; } const startTimestamp = timeOrigin + startTime; @@ -246,7 +253,7 @@ function addResourceSpans( endTimestamp, op: entry.initiatorType && entry.initiatorType !== '' ? `resource.${entry.initiatorType}` : 'resource', startTimestamp, - tags, + data, }); return endTimestamp; diff --git a/packages/tracing/test/browser/metrics.test.ts b/packages/tracing/test/browser/metrics.test.ts index bc4d3f6ee06d..e05177fbae01 100644 --- a/packages/tracing/test/browser/metrics.test.ts +++ b/packages/tracing/test/browser/metrics.test.ts @@ -1,5 +1,5 @@ import { Span, Transaction } from '../../src'; -import { _startChild } from '../../src/browser/metrics'; +import { _startChild, addResourceSpans, ResourceEntry } from '../../src/browser/metrics'; describe('_startChild()', () => { it('creates a span with given properties', () => { @@ -38,3 +38,108 @@ describe('_startChild()', () => { expect(transaction.startTimestamp).toEqual(123); }); }); + +describe('addResourceSpans', () => { + const transaction = new Transaction({ name: 'hello' }); + beforeEach(() => { + transaction.startChild = jest.fn(); + }); + + // We already track xhr, we don't need to use + it('does not create spans for xmlhttprequest', () => { + const entry: ResourceEntry = { + initiatorType: 'xmlhttprequest', + transferSize: 256, + encodedBodySize: 256, + decodedBodySize: 256, + }; + addResourceSpans(transaction, entry, '/assets/to/me', 123, 456, 100); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(transaction.startChild).toHaveBeenCalledTimes(0); + }); + + it('does not create spans for fetch', () => { + const entry: ResourceEntry = { + initiatorType: 'fetch', + transferSize: 256, + encodedBodySize: 256, + decodedBodySize: 256, + }; + addResourceSpans(transaction, entry, '/assets/to/me', 123, 456, 100); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(transaction.startChild).toHaveBeenCalledTimes(0); + }); + + it('creates spans for resource spans', () => { + const entry: ResourceEntry = { + initiatorType: 'css', + transferSize: 256, + encodedBodySize: 456, + decodedBodySize: 593, + }; + + const timeOrigin = 100; + const startTime = 23; + const duration = 356; + + const endTimestamp = addResourceSpans(transaction, entry, '/assets/to/css', startTime, duration, timeOrigin); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(transaction.startChild).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(transaction.startChild).toHaveBeenLastCalledWith({ + data: { + decodedBodySize: entry.decodedBodySize, + encodedBodySize: entry.encodedBodySize, + transferSize: entry.transferSize, + }, + description: '/assets/to/css', + endTimestamp: timeOrigin + startTime + duration, + op: 'resource.css', + startTimestamp: timeOrigin + startTime, + }); + + expect(endTimestamp).toBe(timeOrigin + startTime + duration); + }); + + it('creates a variety of resource spans', () => { + const table = [ + { + initiatorType: undefined, + op: 'resource', + }, + { + initiatorType: '', + op: 'resource', + }, + { + initiatorType: 'css', + op: 'resource.css', + }, + { + initiatorType: 'image', + op: 'resource.image', + }, + { + initiatorType: 'script', + op: 'resource.script', + }, + ]; + + for (const { initiatorType, op } of table) { + const entry: ResourceEntry = { + initiatorType, + }; + addResourceSpans(transaction, entry, '/assets/to/me', 123, 234, 465); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(transaction.startChild).toHaveBeenLastCalledWith( + expect.objectContaining({ + op, + }), + ); + } + }); +}); From 1dffa3eae36e733d099d564063694dc335d72573 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 13 Aug 2020 15:04:09 -0400 Subject: [PATCH 3/6] fix dangerfile --- .eslintrc.js | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .eslintrc.js diff --git a/.eslintrc.js b/.eslintrc.js new file mode 100644 index 000000000000..c15970a3b2b2 --- /dev/null +++ b/.eslintrc.js @@ -0,0 +1,3 @@ +// This is an empty file so that danger works correctly +{ +} From 49a5d6e285393211f5c1609fa7c05d2c35810c39 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 14 Aug 2020 11:27:44 -0400 Subject: [PATCH 4/6] fix: Account for case where they are 0 --- .eslintrc.js | 3 --- packages/tracing/src/browser/metrics.ts | 8 +++---- packages/tracing/test/browser/metrics.test.ts | 24 +++++++++++++++++++ 3 files changed, 28 insertions(+), 7 deletions(-) delete mode 100644 .eslintrc.js diff --git a/.eslintrc.js b/.eslintrc.js deleted file mode 100644 index c15970a3b2b2..000000000000 --- a/.eslintrc.js +++ /dev/null @@ -1,3 +0,0 @@ -// This is an empty file so that danger works correctly -{ -} diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index e7de8efde927..2b725079959c 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -235,13 +235,13 @@ export function addResourceSpans( } const data: Record = {}; - if (entry.transferSize) { + if ('transferSize' in entry) { data.transferSize = entry.transferSize; } - if (entry.encodedBodySize) { + if ('encodedBodySize' in entry) { data.encodedBodySize = entry.encodedBodySize; } - if (entry.decodedBodySize) { + if ('decodedBodySize' in entry) { data.decodedBodySize = entry.decodedBodySize; } @@ -251,7 +251,7 @@ export function addResourceSpans( _startChild(transaction, { description: resourceName, endTimestamp, - op: entry.initiatorType && entry.initiatorType !== '' ? `resource.${entry.initiatorType}` : 'resource', + op: entry.initiatorType ? `resource.${entry.initiatorType}` : 'resource', startTimestamp, data, }); diff --git a/packages/tracing/test/browser/metrics.test.ts b/packages/tracing/test/browser/metrics.test.ts index e05177fbae01..2c4ce61da1dc 100644 --- a/packages/tracing/test/browser/metrics.test.ts +++ b/packages/tracing/test/browser/metrics.test.ts @@ -142,4 +142,28 @@ describe('addResourceSpans', () => { ); } }); + + it('allows for enter size of 0', () => { + const entry: ResourceEntry = { + initiatorType: 'css', + transferSize: 0, + encodedBodySize: 0, + decodedBodySize: 0, + }; + + addResourceSpans(transaction, entry, '/assets/to/css', 100, 23, 345); + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(transaction.startChild).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(transaction.startChild).toHaveBeenLastCalledWith( + expect.objectContaining({ + data: { + decodedBodySize: entry.decodedBodySize, + encodedBodySize: entry.encodedBodySize, + transferSize: entry.transferSize, + }, + }), + ); + }); }); From bfa8cca8140b134d433fca9fe4e3c6e26a8762e5 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 19 Aug 2020 08:50:22 -0400 Subject: [PATCH 5/6] ref: Format data fields --- packages/tracing/src/browser/metrics.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/browser/metrics.ts b/packages/tracing/src/browser/metrics.ts index 2b725079959c..0e3fa26d7fea 100644 --- a/packages/tracing/src/browser/metrics.ts +++ b/packages/tracing/src/browser/metrics.ts @@ -236,13 +236,13 @@ export function addResourceSpans( const data: Record = {}; if ('transferSize' in entry) { - data.transferSize = entry.transferSize; + data['Transfer Size'] = entry.transferSize; } if ('encodedBodySize' in entry) { - data.encodedBodySize = entry.encodedBodySize; + data['Encoded Body Size'] = entry.encodedBodySize; } if ('decodedBodySize' in entry) { - data.decodedBodySize = entry.decodedBodySize; + data['Decoded Body Size'] = entry.decodedBodySize; } const startTimestamp = timeOrigin + startTime; From 63013d6eb23b629d9b605647ea8ea45235f535bd Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 19 Aug 2020 09:16:59 -0400 Subject: [PATCH 6/6] fix: tests --- packages/tracing/test/browser/metrics.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/tracing/test/browser/metrics.test.ts b/packages/tracing/test/browser/metrics.test.ts index 2c4ce61da1dc..f267e2e442dc 100644 --- a/packages/tracing/test/browser/metrics.test.ts +++ b/packages/tracing/test/browser/metrics.test.ts @@ -91,9 +91,9 @@ describe('addResourceSpans', () => { // eslint-disable-next-line @typescript-eslint/unbound-method expect(transaction.startChild).toHaveBeenLastCalledWith({ data: { - decodedBodySize: entry.decodedBodySize, - encodedBodySize: entry.encodedBodySize, - transferSize: entry.transferSize, + ['Decoded Body Size']: entry.decodedBodySize, + ['Encoded Body Size']: entry.encodedBodySize, + ['Transfer Size']: entry.transferSize, }, description: '/assets/to/css', endTimestamp: timeOrigin + startTime + duration, @@ -159,9 +159,9 @@ describe('addResourceSpans', () => { expect(transaction.startChild).toHaveBeenLastCalledWith( expect.objectContaining({ data: { - decodedBodySize: entry.decodedBodySize, - encodedBodySize: entry.encodedBodySize, - transferSize: entry.transferSize, + ['Decoded Body Size']: entry.decodedBodySize, + ['Encoded Body Size']: entry.encodedBodySize, + ['Transfer Size']: entry.transferSize, }, }), );