From 3d1a65db3c78f3de8062f9024b5a085186ec5119 Mon Sep 17 00:00:00 2001 From: Cristian Gonzalez Date: Tue, 16 Jul 2019 13:03:59 -0400 Subject: [PATCH] Refactoring --- .../src/perf-resource-timing-selector.ts | 73 +++++++++---------- .../src/xhr-interceptor.ts | 15 +++- .../test-perf-resource-timing-selector.ts | 11 ++- 3 files changed, 57 insertions(+), 42 deletions(-) diff --git a/packages/opencensus-web-instrumentation-zone/src/perf-resource-timing-selector.ts b/packages/opencensus-web-instrumentation-zone/src/perf-resource-timing-selector.ts index 7d3ab9a8..79b1620a 100644 --- a/packages/opencensus-web-instrumentation-zone/src/perf-resource-timing-selector.ts +++ b/packages/opencensus-web-instrumentation-zone/src/perf-resource-timing-selector.ts @@ -16,6 +16,7 @@ import { Span } from '@opencensus/web-core'; import { XhrPerformanceResourceTiming } from './zone-types'; +import { alreadyAssignedPerfEntries } from './xhr-interceptor'; /** * Get Browser's performance resource timing data associated to a XHR. @@ -32,8 +33,10 @@ export function getXhrPerfomanceData( xhrUrl: string, span: Span ): XhrPerformanceResourceTiming | undefined { - const filteredPerfEntries = getPerfResourceEntries(xhrUrl, span); - const possibleEntries = getPossiblePerfResourceEntries(filteredPerfEntries); + const filteredSortedPerfEntries = getPerfSortedResourceEntries(xhrUrl, span); + const possibleEntries = getPossiblePerfResourceEntries( + filteredSortedPerfEntries + ); const bestEntry = getBestPerfResourceTiming(possibleEntries, span); return bestEntry; } @@ -41,15 +44,18 @@ export function getXhrPerfomanceData( /** * First step for the algorithm. Filter the Performance Resource Timings by the * name (it should match the XHR URL), additionally, the start/end timings of - * every performance entry should fit within the span start/end timings. + * every performance entry should fit within the span start/end timings. Also, + * the entry should be already assigned to a span. * These filtered performance resource entries are considered as possible * entries associated to the xhr. * Those are possible because there might be more than two entries that pass the * filter. + * Additionally, the returned array is sorted by the entries' `startTime` as + * getEntriesByType() already does it. * @param xhrUrl * @param span */ -export function getPerfResourceEntries( +export function getPerfSortedResourceEntries( xhrUrl: string, span: Span ): PerformanceResourceTiming[] { @@ -70,42 +76,36 @@ export function getPerfResourceEntries( * 'paired' with any other entry. * Thus, for this step traverse the array of resource entries and for every * entry check if it is a possible performance resource entry. - * @param perfEntries + * @param filteredSortedPerfEntries Sorted array of Performance Resource + * Entries. This is sorted by the entries' `startTime`. */ export function getPossiblePerfResourceEntries( - perfEntries: PerformanceResourceTiming[] + filteredSortedPerfEntries: PerformanceResourceTiming[] ): XhrPerformanceResourceTiming[] { const possiblePerfEntries = new Array(); - const pairedEntries = new Set(); - let entryI: PerformanceResourceTiming; - let entryJ: PerformanceResourceTiming; // As this part of the algorithm traverses the array twice, although, // this array is not large as the performance resource entries buffer is // cleared when there are no more running XHRs. - for (let i = 0; i < perfEntries.length; i++) { - entryI = perfEntries[i]; - // Compare every performance entry with its consecutive perfomance entries. - // That way to avoid comparing twice the entries. - for (let j = i + 1; j < perfEntries.length; j++) { - entryJ = perfEntries[j]; - if (!overlappingPerfResourceTimings(entryI, entryJ)) { + for (let i = 0; i < filteredSortedPerfEntries.length; i++) { + const entryI = filteredSortedPerfEntries[i]; + // Consider the current entry as a possible entry without cors preflight + // request. This is valid as this entry might be better than other possible + // entries. + possiblePerfEntries.push({ mainRequest: entryI }); + // Compare every performance entry with the perfomance entries in front of + // it. This is possible as the entries are sorted by the startTime. That + // way we to avoid comparing twice the entries and taking the wrong order. + for (let j = i + 1; j < filteredSortedPerfEntries.length; j++) { + const entryJ = filteredSortedPerfEntries[j]; + if (!isPossibleCorsPair(entryI, entryJ)) { // As the entries are not overlapping, that means those timings // are possible perfomance timings related to the XHR. possiblePerfEntries.push({ corsPreFlightRequest: entryI, mainRequest: entryJ, }); - pairedEntries.add(entryI); - pairedEntries.add(entryJ); } } - // If the entry couldn't be paired with any other resource timing, - // add it as a possible resource timing without cors preflight data. - // This is possible because this entry might be better than the other - // possible entries. - if (!pairedEntries.has(entryI)) { - possiblePerfEntries.push({ mainRequest: entryI }); - } } return possiblePerfEntries; } @@ -122,20 +122,16 @@ function getBestPerfResourceTiming( let minimumGapToSpan = Number.MAX_VALUE; let bestPerfEntry: XhrPerformanceResourceTiming | undefined = undefined; for (const perfEntry of perfEntries) { - let gapToSpan = Math.abs( - perfEntry.mainRequest.responseEnd - span.endPerfTime - ); // If the current entry has cors preflight data use its `startTime` to // calculate the gap to the span. - if (perfEntry.corsPreFlightRequest) { - gapToSpan += Math.abs( - perfEntry.corsPreFlightRequest.startTime - span.startPerfTime - ); - } else { - gapToSpan += Math.abs( - perfEntry.mainRequest.startTime - span.startPerfTime - ); - } + const perfEntryStartTime = perfEntry.corsPreFlightRequest + ? perfEntry.corsPreFlightRequest.startTime + : perfEntry.mainRequest.startTime; + const gapToSpan = + span.endPerfTime - + perfEntry.mainRequest.responseEnd + + (perfEntryStartTime - span.startPerfTime); + // If there is a new minimum gap to the span, update the minimum and pick // the current performance entry as the best at this point. if (gapToSpan < minimumGapToSpan) { @@ -152,13 +148,14 @@ function isPerfEntryPartOfXhr( span: Span ): boolean { return ( + !alreadyAssignedPerfEntries.has(entry) && entry.name === xhrUrl && entry.startTime >= span.startPerfTime && entry.responseEnd <= span.endPerfTime ); } -function overlappingPerfResourceTimings( +function isPossibleCorsPair( entry1: PerformanceResourceTiming, entry2: PerformanceResourceTiming ): boolean { diff --git a/packages/opencensus-web-instrumentation-zone/src/xhr-interceptor.ts b/packages/opencensus-web-instrumentation-zone/src/xhr-interceptor.ts index d0fbc622..97244dfe 100644 --- a/packages/opencensus-web-instrumentation-zone/src/xhr-interceptor.ts +++ b/packages/opencensus-web-instrumentation-zone/src/xhr-interceptor.ts @@ -44,6 +44,11 @@ const xhrSpans = new Map(); // xhr tasks are being intercepted. let xhrTasksCount = 0; +// Set to keep track of already assigned performance resource entries to a span. +// This is done as there might be some edge cases where the result might include +// some already assigned entries. +export const alreadyAssignedPerfEntries = new Set(); + /** * Intercepts task as XHR if it is a tracked task and its target object is * instance of `XMLHttpRequest`. @@ -108,13 +113,21 @@ function endXhrSpan(xhr: XhrWithUrl): void { // This is done in order to help the browser Performance resource timings // selector algorithm to take only the data related to the current XHRs running. function maybeClearPerfResourceBuffer(): void { - if (xhrTasksCount === 0) performance.clearResourceTimings(); + if (xhrTasksCount === 0) { + performance.clearResourceTimings(); + // Clear the set as these entries are not longer necessary. + alreadyAssignedPerfEntries.clear(); + } } function joinPerfResourceDataToSpan(xhr: XhrWithUrl, span: Span) { const xhrPerfResourceTiming = getXhrPerfomanceData(xhr.responseURL, span); if (xhrPerfResourceTiming) { + alreadyAssignedPerfEntries.add(xhrPerfResourceTiming.mainRequest); if (xhrPerfResourceTiming.corsPreFlightRequest) { + alreadyAssignedPerfEntries.add( + xhrPerfResourceTiming.corsPreFlightRequest + ); const corsPerfTiming = xhrPerfResourceTiming.corsPreFlightRequest as PerformanceResourceTimingExtended; setCorsPerfTimingAsChildSpan(corsPerfTiming, span); } diff --git a/packages/opencensus-web-instrumentation-zone/test/test-perf-resource-timing-selector.ts b/packages/opencensus-web-instrumentation-zone/test/test-perf-resource-timing-selector.ts index 75d66e37..dc2606f1 100644 --- a/packages/opencensus-web-instrumentation-zone/test/test-perf-resource-timing-selector.ts +++ b/packages/opencensus-web-instrumentation-zone/test/test-perf-resource-timing-selector.ts @@ -16,7 +16,7 @@ import { Span } from '@opencensus/web-core'; import { - getPerfResourceEntries, + getPerfSortedResourceEntries, getPossiblePerfResourceEntries, getXhrPerfomanceData, } from '../src/perf-resource-timing-selector'; @@ -55,7 +55,7 @@ describe('Perf Resource Timing Selector', () => { const span = createSpan(12, 30); const entriesToBeFiltered = [entry2, entry3]; - const filteredPerfEntries = getPerfResourceEntries('/test', span); + const filteredPerfEntries = getPerfSortedResourceEntries('/test', span); expect(filteredPerfEntries).toEqual(entriesToBeFiltered); }); }); @@ -92,10 +92,15 @@ describe('Perf Resource Timing Selector', () => { mainRequest: entry3, } as XhrPerformanceResourceTiming; - expect(filteredPerfEntries.length).toBe(2); + expect(filteredPerfEntries.length).toBe(5); expect(filteredPerfEntries).toContain(nonOverlappingEntry1); expect(filteredPerfEntries).toContain(nonOverlappingEntry2); expect(filteredPerfEntries).not.toContain(overlappingEntry3); + // As every entry is considered a possible Xhr performance entry + // without CORS preflight, check the should be present. + expect(filteredPerfEntries).toContain({ mainRequest: entry1 }); + expect(filteredPerfEntries).toContain({ mainRequest: entry2 }); + expect(filteredPerfEntries).toContain({ mainRequest: entry3 }); }); it('Should not add cors preflight value as all the entries overlap each other', () => {