Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

telemetry(amazonq): attach stack trace in onCodeGeneration failures #6293

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
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
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import { getWorkspaceFoldersByPrefixes } from '../../../shared/utilities/workspa
import { openDeletedDiff, openDiff } from '../../../amazonq/commons/diff'
import { i18n } from '../../../shared/i18n-helper'
import globals from '../../../shared/extensionGlobals'
import { randomUUID } from '../../../shared'
import { getStackTraceForError, randomUUID } from '../../../shared'
import { FollowUpTypes } from '../../../amazonq/commons/types'
import { Messenger } from '../../../amazonq/commons/connector/baseMessenger'
import { BaseChatSessionStorage } from '../../../amazonq/commons/baseChatStorage'
Expand Down Expand Up @@ -504,6 +504,10 @@ export class FeatureDevController {
result = MetricDataResult.Fault
}
break
case ContentLengthError.name:
case MonthlyConversationLimitError.name:
case CodeIterationLimitError.name:
case UploadURLExpired.name:
case PromptRefusalException.name:
case NoChangeRequiredException.name:
result = MetricDataResult.Error
Expand All @@ -513,7 +517,11 @@ export class FeatureDevController {
break
}

await session.sendMetricDataTelemetry(MetricDataOperationName.EndCodeGeneration, result)
await session.sendMetricDataTelemetry(
MetricDataOperationName.EndCodeGeneration,
result,
'stack trace: ' + getStackTraceForError(err)
)
throw err
} finally {
// Finish processing the event
Expand Down
33 changes: 17 additions & 16 deletions packages/core/src/amazonqFeatureDev/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,112 +3,113 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { ToolkitError } from '../shared/errors'
import { SafeMessageToolkitError, ToolkitError } from '../shared/errors'
import { featureName } from './constants'
import { uploadCodeError } from './userFacingText'
import { i18n } from '../shared/i18n-helper'

export class ConversationIdNotFoundError extends ToolkitError {
export class ConversationIdNotFoundError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.conversationIdNotFoundError'), {
code: 'ConversationIdNotFound',
})
}
}

export class TabIdNotFoundError extends ToolkitError {
export class TabIdNotFoundError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.tabIdNotFoundError'), {
code: 'TabIdNotFound',
})
}
}

export class WorkspaceFolderNotFoundError extends ToolkitError {
export class WorkspaceFolderNotFoundError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.workspaceFolderNotFoundError'), {
code: 'WorkspaceFolderNotFound',
})
}
}

export class UserMessageNotFoundError extends ToolkitError {
export class UserMessageNotFoundError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.userMessageNotFoundError'), {
code: 'MessageNotFound',
})
}
}

export class SelectedFolderNotInWorkspaceFolderError extends ToolkitError {
export class SelectedFolderNotInWorkspaceFolderError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.selectedFolderNotInWorkspaceFolderError'), {
code: 'SelectedFolderNotInWorkspaceFolder',
})
}
}

export class PromptRefusalException extends ToolkitError {
export class PromptRefusalException extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.promptRefusalException'), {
code: 'PromptRefusalException',
})
}
}

export class NoChangeRequiredException extends ToolkitError {
export class NoChangeRequiredException extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.noChangeRequiredException'), {
code: 'NoChangeRequiredException',
})
}
}

export class FeatureDevServiceError extends ToolkitError {
// To prevent potential security issues, message passed in should be predictably safe for telemetry
export class FeatureDevServiceError extends SafeMessageToolkitError {
constructor(message: string, code: string) {
super(message, { code })
}
}

export class PrepareRepoFailedError extends ToolkitError {
export class PrepareRepoFailedError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.prepareRepoFailedError'), {
code: 'PrepareRepoFailed',
})
}
}

export class UploadCodeError extends ToolkitError {
export class UploadCodeError extends SafeMessageToolkitError {
constructor(statusCode: string) {
super(uploadCodeError, { code: `UploadCode-${statusCode}` })
}
}

export class UploadURLExpired extends ToolkitError {
export class UploadURLExpired extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.uploadURLExpired'), { code: 'UploadURLExpired' })
}
}

export class IllegalStateTransition extends ToolkitError {
export class IllegalStateTransition extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.illegalStateTransition'), { code: 'IllegalStateTransition' })
}
}

export class ContentLengthError extends ToolkitError {
export class ContentLengthError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.contentLengthError'), { code: ContentLengthError.name })
}
}

export class ZipFileError extends ToolkitError {
export class ZipFileError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.zipFileError'), { code: ZipFileError.name })
}
}

export class CodeIterationLimitError extends ToolkitError {
export class CodeIterationLimitError extends SafeMessageToolkitError {
constructor() {
super(i18n('AWS.amazonq.featureDev.error.codeIterationLimitError'), { code: CodeIterationLimitError.name })
}
Expand Down
12 changes: 11 additions & 1 deletion packages/core/src/amazonqFeatureDev/session/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ export class Session {
return { leftPath, rightPath, ...diff }
}

public async sendMetricDataTelemetry(operationName: string, result: string) {
public async sendMetricDataTelemetry(operationName: string, result: string, log?: string) {
await this.proxyClient.sendMetricData({
metricName: 'Operation',
metricValue: 1,
Expand All @@ -300,6 +300,16 @@ export class Session {
name: 'result',
value: result,
},

// The log dimension will be emitted only to CloudWatch Logs
...(log
? [
{
name: 'log',
value: log,
},
]
: []),
],
})
}
Expand Down
51 changes: 51 additions & 0 deletions packages/core/src/shared/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,57 @@ export function getTelemetryResult(error: unknown | undefined): Result {
return 'Failed'
}

export class SafeMessageToolkitError extends ToolkitError {
public constructor(message: string, info: ErrorInformation = {}) {
super(message, info)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is going on here? why was this new base class introduced? what is the intended meaning of "safe message" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Safe message means the error message is safe to send to telemetry (no critical information). This was introduced to allow error classes which extends it can be sent to telemetry with its error message.
But this PR is not ready yet, so I will draft it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't sound like the right direction. We already have scrubNames

export function scrubNames(s: string, username?: string) {

which removes PII from strings. And that is used by getTelemetryReasonDesc, which already is used automatically.

Adding new, special classes is not the right way to approach this.

}
}

/**
* This function constructs a string similar to error.printStackTrace() for telemetry,
* but include error messages only for safe exceptions,
* i.e. exceptions with deterministic error messages and do not include sensitive data.
*
* @param error The error to get stack trace string from
*/
export function getStackTraceForError(error: Error): string {
const recursionLimit = 3
const seenExceptions = new Set<Error>()
const lines: string[] = []

function printExceptionDetails(err: Error, depth: number, prefix: string = '') {
if (depth >= recursionLimit || seenExceptions.has(err)) {
return
}
seenExceptions.add(err)

if (error instanceof SafeMessageToolkitError) {
lines.push(`${prefix}${err.constructor.name}: ${err.message}`)
} else {
lines.push(`${prefix}${err.constructor.name}`)
}

if (err.stack) {
const startStr = err.stack.substring('Error: '.length)
const callStack = startStr.substring(startStr.indexOf(err.message) + err.message.length + 1)
for (const line of callStack.split('\n')) {
const scrubbed = scrubNames(line, _username)
lines.push(`${prefix}\t${scrubbed}`)
}
}

const cause = (err as any).cause
if (cause instanceof Error) {
lines.push(`${prefix}\tCaused by: `)
printExceptionDetails(cause, depth + 1, `${prefix}\t`)
}
}

printExceptionDetails(error, 0)
getLogger().info(lines.join('\n'))
return lines.join('\n')
}

/**
* Removes potential PII from a string, for logging/telemetry.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,10 @@ describe('Controller', () => {

const errorResultMapping = new Map([
['EmptyPatchException', MetricDataResult.LlmFailure],
[ContentLengthError.name, MetricDataResult.Error],
[MonthlyConversationLimitError.name, MetricDataResult.Error],
[CodeIterationLimitError.name, MetricDataResult.Error],
[UploadURLExpired.name, MetricDataResult.Error],
[PromptRefusalException.name, MetricDataResult.Error],
[NoChangeRequiredException.name, MetricDataResult.Error],
])
Expand Down Expand Up @@ -519,7 +523,16 @@ describe('Controller', () => {
)
const metricResult = getMetricResult(error)
assert.ok(
sendMetricDataTelemetrySpy.calledWith(MetricDataOperationName.EndCodeGeneration, metricResult)
sendMetricDataTelemetrySpy.calledWith(
MetricDataOperationName.EndCodeGeneration,
metricResult,
sinon.match(
(str) =>
typeof str === 'string' &&
str.includes('stack trace: ' + error.constructor.name) &&
str.includes('at ')
)
)
)
}

Expand Down
47 changes: 47 additions & 0 deletions packages/core/src/test/shared/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import {
tryRun,
UnknownError,
getErrorId,
getStackTraceForError,
SafeMessageToolkitError,
} from '../../shared/errors'
import { CancellationError } from '../../shared/utilities/timeoutUtils'
import { UnauthorizedException } from '@aws-sdk/client-sso'
Expand Down Expand Up @@ -347,6 +349,51 @@ describe('Telemetry', function () {
assert.strictEqual(getTelemetryReason(error2), 'ErrorCode')
})
})

describe('getStackTraceForError', () => {
class SafeError extends SafeMessageToolkitError {}
class UnsafeError extends ToolkitError {}

it('includes message for safe exceptions', () => {
const safeError = new SafeError('Safe error message')
const result = getStackTraceForError(safeError)

assert.ok(result.includes('Safe error message'))
assert.ok(result.includes('SafeError: '))
})

it('excludes message for unsafe exceptions', () => {
const unsafeError = new UnsafeError('Sensitive information')
const result = getStackTraceForError(unsafeError)

assert.ok(!result.includes('Sensitive information'))
assert.ok(result.includes('UnsafeError'))
})

it('respects recursion limit for nested exceptions', () => {
const recursionLimit = 3
const exceptions: ToolkitError[] = []

let currentError = new SafeError(`depth ${recursionLimit + 2}`)
exceptions.push(currentError)

for (let i = recursionLimit + 1; i >= 0; i--) {
currentError = new SafeError(`depth ${i}`, { cause: currentError })
exceptions.push(currentError)
}
const stackTrace = getStackTraceForError(exceptions[exceptions.length - 1])

// Assert exceptions within limit are included
for (let i = 0; i < recursionLimit; i++) {
assert(stackTrace.includes(`depth ${i}`), `Stack trace should include error at depth ${i}`)
}

// Assert exceptions beyond limit are not included
for (let i = recursionLimit; i <= exceptions.length; i++) {
assert(!stackTrace.includes(`depth ${i}`), `Stack trace should not include error at depth ${i}`)
}
})
})
})

describe('resolveErrorMessageToDisplay()', function () {
Expand Down
Loading