Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"generateNonCodeFiles": "npm run generateNonCodeFiles -w packages/ --if-present"
},
"devDependencies": {
"@aws-toolkits/telemetry": "^1.0.258",
"@aws-toolkits/telemetry": "^1.0.267",
"@playwright/browser-chromium": "^1.43.1",
"@types/vscode": "^1.68.0",
"@types/vscode-webview": "^1.57.1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import {
maxRepoSizeBytes,
} from 'aws-core-vscode/amazonqFeatureDev'
import { assertTelemetry, createTestWorkspace } from 'aws-core-vscode/test'
import { fs, AmazonqCreateUpload, Metric } from 'aws-core-vscode/shared'
import { fs, AmazonqCreateUpload } from 'aws-core-vscode/shared'
import { Span } from 'aws-core-vscode/telemetry'
import sinon from 'sinon'

describe('file utils', () => {
Expand All @@ -28,7 +29,7 @@ describe('file utils', () => {
const telemetry = new TelemetryHelper()
const result = await prepareRepoData([workspace.uri.fsPath], [workspace], telemetry, {
record: () => {},
} as unknown as Metric<AmazonqCreateUpload>)
} as unknown as Span<AmazonqCreateUpload>)
assert.strictEqual(Buffer.isBuffer(result.zipFileBuffer), true)
// checksum is not the same across different test executions because some unique random folder names are generated
assert.strictEqual(result.zipFileChecksum.length, 44)
Expand All @@ -46,7 +47,7 @@ describe('file utils', () => {
const telemetry = new TelemetryHelper()
const result = await prepareRepoData([workspace.uri.fsPath], [workspace], telemetry, {
record: () => {},
} as unknown as Metric<AmazonqCreateUpload>)
} as unknown as Span<AmazonqCreateUpload>)

assert.strictEqual(Buffer.isBuffer(result.zipFileBuffer), true)
// checksum is not the same across different test executions because some unique random folder names are generated
Expand All @@ -65,7 +66,7 @@ describe('file utils', () => {
() =>
prepareRepoData([workspace.uri.fsPath], [workspace], telemetry, {
record: () => {},
} as unknown as Metric<AmazonqCreateUpload>),
} as unknown as Span<AmazonqCreateUpload>),
ContentLengthError
)
})
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/amazonqFeatureDev/util/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { maxFileSizeBytes } from '../limits'
import { createHash } from 'crypto'
import { CurrentWsFolders } from '../types'
import { ToolkitError } from '../../shared/errors'
import { AmazonqCreateUpload, Metric, telemetry as amznTelemetry } from '../../shared/telemetry/telemetry'
import { AmazonqCreateUpload, Span, telemetry as amznTelemetry } from '../../shared/telemetry/telemetry'
import { TelemetryHelper } from './telemetryHelper'
import { maxRepoSizeBytes } from '../constants'
import { isCodeFile } from '../../shared/filetypes'
Expand All @@ -28,7 +28,7 @@ export async function prepareRepoData(
repoRootPaths: string[],
workspaceFolders: CurrentWsFolders,
telemetry: TelemetryHelper,
span: Metric<AmazonqCreateUpload>
span: Span<AmazonqCreateUpload>
) {
try {
const files = await collectFiles(repoRootPaths, workspaceFolders, true, maxRepoSizeBytes)
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/amazonqFeatureDev/util/telemetryHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { globals } from '../../shared'
import { getLogger } from '../../shared/logger/logger'
import { AmazonqApproachInvoke, AmazonqCodeGenerationInvoke, Metric } from '../../shared/telemetry/telemetry'
import { AmazonqApproachInvoke, AmazonqCodeGenerationInvoke, Span } from '../../shared/telemetry/telemetry'
import { LLMResponseType } from '../types'

export class TelemetryHelper {
Expand All @@ -32,7 +32,7 @@ export class TelemetryHelper {
}

public recordUserApproachTelemetry(
span: Metric<AmazonqApproachInvoke>,
span: Span<AmazonqApproachInvoke>,
amazonqConversationId: string,
responseType: LLMResponseType
) {
Expand All @@ -48,7 +48,7 @@ export class TelemetryHelper {
span.record(event)
}

public recordUserCodeGenerationTelemetry(span: Metric<AmazonqCodeGenerationInvoke>, amazonqConversationId: string) {
public recordUserCodeGenerationTelemetry(span: Span<AmazonqCodeGenerationInvoke>, amazonqConversationId: string) {
const event = {
amazonqConversationId,
amazonqGenerateCodeIteration: this.generateCodeIteration,
Expand Down
12 changes: 6 additions & 6 deletions packages/core/src/auth/secondaryAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { isNonNullable } from '../shared/utilities/tsUtils'
import { ToolIdStateKey } from '../shared/globalState'
import { Connection, getTelemetryMetadataForConn, SsoConnection, StatefulConnection } from './connection'
import { indent } from '../shared/utilities/textUtilities'
import { AuthStatus, telemetry } from '../shared/telemetry/telemetry'
import { AuthModifyConnection, AuthStatus, Span, telemetry } from '../shared/telemetry/telemetry'
import { asStringifiedStack } from '../shared/telemetry/spans'
import { withTelemetryContext } from '../shared/telemetry/util'
import { isNetworkError } from '../shared/errors'
Expand Down Expand Up @@ -307,7 +307,7 @@ export class SecondaryAuth<T extends Connection = Connection> {
connectionState: 'undefined',
})
await this.auth.tryAutoConnect()
this.#savedConnection = await this._loadSavedConnection()
this.#savedConnection = await this._loadSavedConnection(span)
this.#onDidChangeActiveConnection.fire(this.activeConnection)

const conn = this.#savedConnection
Expand All @@ -328,7 +328,7 @@ export class SecondaryAuth<T extends Connection = Connection> {
/**
* Provides telemetry if called by restoreConnection() (or another auth_modifyConnection context)
*/
private async _loadSavedConnection() {
private async _loadSavedConnection(span: Span<AuthModifyConnection>) {
const id = this.getStateConnectionId()
if (id === undefined) {
return
Expand All @@ -349,7 +349,7 @@ export class SecondaryAuth<T extends Connection = Connection> {
let connectionState = this.auth.getConnectionState(conn)

// This function is expected to be called in the context of restoreConnection()
telemetry.auth_modifyConnection.record({
span.record({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for reference: this could also do telemetry.record() , right? In general, I don't think we can (or want to) except spans to be passed around; that defeats much of the advantage of the "execution scope" concept, which allows the context to be augmented by any codepath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yes, but it also has the side effect that every subsequent span thats created would have connectionState and authStatus

Instead I could have technically used telemetry.activeSpan?.record()

connectionState,
authStatus: getAuthStatus(connectionState),
})
Expand All @@ -358,7 +358,7 @@ export class SecondaryAuth<T extends Connection = Connection> {
await this.auth.refreshConnectionState(conn)

connectionState = this.auth.getConnectionState(conn)
telemetry.auth_modifyConnection.record({
span.record({
connectionState,
authStatus: getAuthStatus(connectionState),
})
Expand All @@ -367,7 +367,7 @@ export class SecondaryAuth<T extends Connection = Connection> {
// If updating the state fails, then we should delegate downstream to handle getting the proper state.
getLogger().error('loadSavedConnection: Failed to refresh connection state: %s', err)
if (isNetworkError(err) && connectionState === 'valid') {
telemetry.auth_modifyConnection.record({
span.record({
authStatus: 'connectedWithNetworkError',
})
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/shared/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
*/

export { activate } from './activation'
export { telemetry, AuthStatus } from './telemetry'
export { telemetry, AuthStatus, Span } from './telemetry'
export { ExtStartUpSources } from './util'
18 changes: 7 additions & 11 deletions packages/core/src/shared/telemetry/spans.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
MetricDefinition,
MetricName,
MetricShapes,
Span,
TelemetryBase,
} from './telemetry.gen'
import {
Expand Down Expand Up @@ -132,7 +133,7 @@ export type SpanOptions = {
*
* See also: docs/telemetry.md
*/
export class TelemetrySpan<T extends MetricBase = MetricBase> {
export class TelemetrySpan<T extends MetricBase = MetricBase> implements Span<T> {
#startTime?: Date
#options: SpanOptions

Expand Down Expand Up @@ -317,6 +318,9 @@ export class TelemetryTracer extends TelemetryBase {
* All changes made to {@link attributes} (via {@link record}) during the execution are
* reverted after the execution completes.
*
* Runs can be nested within each other. This allows for creating hierarchical spans,
* where child spans inherit the context of their parent spans.
*
* This method automatically handles traceId generation and propagation:
* - If no traceId exists in the current context, a new one is generated.
* - The traceId is attached to all telemetry events created within this span.
Expand All @@ -325,19 +329,13 @@ export class TelemetryTracer extends TelemetryBase {
*
* See docs/telemetry.md
*/
public run<T, U extends MetricName>(name: U, fn: (span: Metric<MetricShapes[U]>) => T, options?: SpanOptions): T {
public run<T, U extends MetricName>(name: U, fn: (span: Span<MetricShapes[U]>) => T, options?: SpanOptions): T {
return this.withTraceId(() => {
const span = this.createSpan(name, options).start()
const frame = this.switchContext(span)

try {
//
// TODO: Since updating to `@types/node@16`, typescript flags this code with error:
//
// Error: npm ERR! src/shared/telemetry/spans.ts(255,57): error TS2345: Argument of type
// 'TelemetrySpan<MetricBase>' is not assignable to parameter of type 'Metric<MetricShapes[U]>'.
//
const result = this.#context.run(frame, fn, span as any)
const result = this.#context.run(frame, fn, span)

if (result instanceof Promise) {
return result
Expand Down Expand Up @@ -444,9 +442,7 @@ export class TelemetryTracer extends TelemetryBase {
return {
name,
emit: (data) => getSpan().emit(data),
record: (data) => getSpan().record(data),
run: (fn, options?: SpanOptions) => this.run(name as MetricName, fn, options),
increment: (data) => getSpan().increment(data),
}
}

Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/shared/telemetry/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ export const telemetry = new TelemetryTracer()
*/
declare module './telemetry.gen' {
interface Metric<T extends MetricBase = MetricBase> {
run<U>(fn: (span: Span<T>) => U, options?: SpanOptions): U
}

interface Span<T extends MetricBase = MetricBase> {
increment(data: { [P in NumericKeys<T>]+?: number }): void
run<U>(fn: (span: this) => U, options?: SpanOptions): U
}
Comment on lines 25 to 31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't span have run()? I don't quite understand the distinction here. I think of metrics as mostly synonymous with spans.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is this span as any is a lie. It never actually had run. When you try and do span.run you get "span.run is not a function"

The passed in span is just a telemetry span that has emit, record, increment but isn't a metric type

}
4 changes: 2 additions & 2 deletions packages/core/src/shared/vscode/commands2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { toTitleCase } from '../utilities/textUtilities'
import { getLogger, NullLogger } from '../logger/logger'
import { FunctionKeys, Functions, getFunctions } from '../utilities/classUtils'
import { TreeItemContent, TreeNode } from '../treeview/resourceTreeDataProvider'
import { telemetry, MetricName, VscodeExecuteCommand, Metric } from '../telemetry/telemetry'
import { telemetry, MetricName, VscodeExecuteCommand, Metric, Span } from '../telemetry/telemetry'
import globals from '../extensionGlobals'
import { ToolkitError } from '../errors'
import crypto from 'crypto'
Expand Down Expand Up @@ -498,7 +498,7 @@ function getInstrumenter(
return <T extends Callback>(fn: T, ...args: Parameters<T>) =>
span.run(
(span) => {
;(span as Metric<VscodeExecuteCommand>).record({
;(span as Span<VscodeExecuteCommand>).record({
command: id.id,
debounceCount,
...fields,
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/test/shared/telemetry/spans.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ describe('TelemetryTracer', function () {

it('does not change the active span when using a different span', function () {
tracer.run(metricName, (span) => {
tracer.vscode_executeCommand.record({ command: 'foo', debounceCount: 1 })
tracer.vscode_executeCommand.emit({ command: 'foo', debounceCount: 1 })
assert.strictEqual(tracer.activeSpan, span)
})

Expand Down
4 changes: 0 additions & 4 deletions packages/core/src/threatComposer/threatComposerEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ export class ThreatComposerEditor {
this.defaultTemplateName = path.basename(this.defaultTemplatePath)
this.fileId = fileId

telemetry.threatComposer_opened.record({
id: this.fileId,
})

this.setupWebviewPanel(textDocument, context)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export class ThreatComposerEditorProvider implements vscode.CustomTextEditorProv
this.getWebviewContent
)
this.handleNewVisualization(document.uri.fsPath, newVisualization)
span.record({ id: fileId })
} catch (err) {
this.handleErr(err as Error)
throw new ToolkitError((err as Error).message, { code: 'Failed to Open Threat Composer' })
Expand Down
Loading