Skip to content

Commit

Permalink
refactor(core): Remove trackPerformance from telemetry (#5708)
Browse files Browse the repository at this point in the history
## Problem
- withPerformance was an experiment to see if we could correlate
cpu/memory with number of files


## Solution
- Cpu usage/memory usage in the extension host is way too volitile so we
might as well clean up this code


---

<!--- REMINDER: Ensure that your PR meets the guidelines in
CONTRIBUTING.md -->

License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.
  • Loading branch information
jpinkney-aws authored Oct 2, 2024
1 parent 1e462f9 commit 762b7b4
Show file tree
Hide file tree
Showing 6 changed files with 2 additions and 98 deletions.
5 changes: 0 additions & 5 deletions packages/core/src/shared/performance/performance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import assert from 'assert'
import { getLogger } from '../logger'
import { isWeb } from '../extensionGlobals'

interface PerformanceMetrics {
/**
Expand Down Expand Up @@ -46,10 +45,6 @@ export class PerformanceTracker {

constructor(private readonly name: string) {}

static enabled(name: string, trackPerformance: boolean): boolean {
return name === 'function_call' && trackPerformance && !isWeb()
}

start() {
this.#startPerformance = {
cpuUsage: process.cpuUsage(),
Expand Down
29 changes: 1 addition & 28 deletions packages/core/src/shared/telemetry/spans.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
getTelemetryResult,
} from '../errors'
import { entries, NumericKeys } from '../utilities/tsUtils'
import { PerformanceTracker } from '../performance/performance'
import { randomUUID } from '../crypto'

const AsyncLocalStorage: typeof AsyncLocalStorageClass =
Expand Down Expand Up @@ -135,10 +134,7 @@ export type SpanOptions = {
*/
export class TelemetrySpan<T extends MetricBase = MetricBase> {
#startTime?: Date
#options: SpanOptions & {
trackPerformance: boolean
}
#performance?: PerformanceTracker
#options: SpanOptions

private readonly state: Partial<T> = {}
private readonly definition = definitions[this.name] ?? {
Expand All @@ -163,10 +159,6 @@ export class TelemetrySpan<T extends MetricBase = MetricBase> {
// do emit by default
emit: options?.emit === undefined ? true : options.emit,
functionId: options?.functionId,
trackPerformance: PerformanceTracker.enabled(
this.name,
this.definition.trackPerformance && (options?.emit ?? false) // only track the performance if we are also emitting
),
}

this.metricId = randomUUID()
Expand Down Expand Up @@ -209,10 +201,6 @@ export class TelemetrySpan<T extends MetricBase = MetricBase> {
*/
public start(): this {
this.#startTime = new globals.clock.Date()
if (this.#options.trackPerformance) {
;(this.#performance ??= new PerformanceTracker(this.name)).start()
}

return this
}

Expand All @@ -224,21 +212,6 @@ export class TelemetrySpan<T extends MetricBase = MetricBase> {
public stop(err?: unknown): void {
const duration = this.startTime !== undefined ? globals.clock.Date.now() - this.startTime.getTime() : undefined

if (this.#options.trackPerformance) {
// TODO add these to the global metrics, right now it just forces them in the telemetry and ignores the type
// if someone enables this action
const performanceMetrics = this.#performance?.stop()
if (performanceMetrics) {
this.record({
userCpuUsage: performanceMetrics.userCpuUsage,
systemCpuUsage: performanceMetrics.systemCpuUsage,
heapTotal: performanceMetrics.heapTotal,
functionName: this.#options.functionId?.name ?? this.name,
architecture: process.arch,
} as any)
}
}

if (this.#options.emit) {
this.emit({
duration,
Expand Down
21 changes: 1 addition & 20 deletions packages/core/src/shared/telemetry/vscodeTelemetry.json
Original file line number Diff line number Diff line change
Expand Up @@ -307,16 +307,6 @@
"type": "string",
"description": "The CPU architecture"
},
{
"name": "totalFileSizeInMB",
"type": "int",
"description": "The total size of all files being sent to Amazon Q"
},
{
"name": "totalFiles",
"type": "int",
"description": "The total number of files being sent to Amazon Q"
},
{
"name": "traceId",
"type": "string",
Expand Down Expand Up @@ -1165,14 +1155,6 @@
"type": "systemCpuUsage",
"required": false
},
{
"type": "totalFiles",
"required": false
},
{
"type": "totalFileSizeInMB",
"required": false
},
{
"type": "architecture",
"required": false
Expand All @@ -1186,8 +1168,7 @@
"required": true
}
],
"passive": true,
"trackPerformance": true
"passive": true
},
{
"name": "webview_load",
Expand Down
3 changes: 0 additions & 3 deletions packages/core/src/shared/utilities/workspaceUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,15 +344,13 @@ export async function collectFiles(
}

let totalSizeBytes = 0
let totalFiles = 0
for (const rootPath of sourcePaths) {
const allFiles = await vscode.workspace.findFiles(
new vscode.RelativePattern(rootPath, '**'),
getExcludePattern()
)

const files = respectGitIgnore ? await filterOutGitignoredFiles(rootPath, allFiles) : allFiles
totalFiles += files.length

for (const file of files) {
const relativePath = getWorkspaceRelativePath(file.fsPath, { workspaceFolders })
Expand Down Expand Up @@ -385,7 +383,6 @@ export async function collectFiles(
})
}
}
span.record({ totalFiles, totalFileSizeInMB: totalSizeBytes / (1024 * 1024) })
return storage
},
{
Expand Down
40 changes: 0 additions & 40 deletions packages/core/src/test/shared/telemetry/spans.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { sleep } from '../../../shared'
import { withTelemetryContext } from '../../../shared/telemetry/util'
import { SinonSandbox } from 'sinon'
import sinon from 'sinon'
import { stubPerformance } from '../../utilities/performance'
import * as crypto from '../../../shared/crypto'

describe('TelemetrySpan', function () {
Expand Down Expand Up @@ -77,23 +76,6 @@ describe('TelemetrySpan', function () {
{ result: 'Succeeded', duration: 100 },
])
})

it('records performance', function () {
const { expectedUserCpuUsage, expectedSystemCpuUsage, expectedHeapTotal } = stubPerformance(sandbox)
const span = new TelemetrySpan('function_call', {
emit: true,
})
span.start()
clock.tick(90)
span.stop()
assertTelemetry('function_call', {
userCpuUsage: expectedUserCpuUsage,
systemCpuUsage: expectedSystemCpuUsage,
heapTotal: expectedHeapTotal,
duration: 90,
result: 'Succeeded',
})
})
})

describe('TelemetryTracer', function () {
Expand Down Expand Up @@ -269,28 +251,6 @@ describe('TelemetryTracer', function () {
assert.match(metric.requestId ?? '', /[a-z0-9-]+/)
})

it('records performance', function () {
clock = installFakeClock()
const { expectedUserCpuUsage, expectedSystemCpuUsage, expectedHeapTotal } = stubPerformance(sandbox)
tracer.run(
'function_call',
() => {
clock?.tick(90)
},
{
emit: true,
}
)

assertTelemetry('function_call', {
userCpuUsage: expectedUserCpuUsage,
systemCpuUsage: expectedSystemCpuUsage,
heapTotal: expectedHeapTotal,
duration: 90,
result: 'Succeeded',
})
})

describe('nested run()', function () {
let uuidStub: sinon.SinonStub
const nestedName = 'nested_metric' as MetricName
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,6 @@ describe('collectFiles', function () {
)

assertTelemetry('function_call', {
totalFiles: 8,
totalFileSizeInMB: 0.0002593994140625,
functionName: 'collectFiles',
})
})
Expand Down

0 comments on commit 762b7b4

Please sign in to comment.