Skip to content

Commit 4bb2fc1

Browse files
refactor(telemetry): prepend the error code/name to the reasonDesc field (#5557)
## Problem: In telemetry we sometimes add the error message to the `reasonDesc` field. Additionally when using `getTelemetryReasonDesc()` it will recurse through the causal chain of the error and return a value composed of all the error messages. Eg: `Reason A | Reason B | Reason C` The issue is that the nested errors may have useful context (in addition to the message) in their id (code or name). And currently this is lost. ## Solution: Prepend the code/name to the error messages when using `getTelemetryReasonDesc()` Eg: `CodeA: Reason A | CodeB: Reason B | NameC: Reason C` --- <!--- 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. --------- Signed-off-by: Nikolas Komonen <[email protected]>
1 parent 4a88c9e commit 4bb2fc1

File tree

4 files changed

+69
-21
lines changed

4 files changed

+69
-21
lines changed

packages/core/src/shared/errors.ts

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -251,16 +251,11 @@ export class ToolkitError extends Error implements ErrorInformation {
251251
* @param withCause Append the message(s) from the cause chain, recursively.
252252
* The message(s) are delimited by ' | '. Eg: msg1 | causeMsg1 | causeMsg2
253253
*/
254-
export function getErrorMsg(err: Error | undefined, withCause = false): string | undefined {
254+
export function getErrorMsg(err: Error | undefined, withCause: boolean = false): string | undefined {
255255
if (err === undefined) {
256256
return undefined
257257
}
258258

259-
const cause = (err as any).cause
260-
if (withCause && cause) {
261-
return `${err.message}${cause ? ' | ' + getErrorMsg(cause, true) : ''}`
262-
}
263-
264259
// Non-standard SDK fields added by the OIDC service, to conform to the OAuth spec
265260
// (https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1) :
266261
// - error: code per the OAuth spec
@@ -286,12 +281,25 @@ export function getErrorMsg(err: Error | undefined, withCause = false): string |
286281
// }
287282
const anyDesc = (err as any).error_description
288283
const errDesc = typeof anyDesc === 'string' ? anyDesc.trim() : ''
289-
const msg = errDesc !== '' ? errDesc : err.message?.trim()
284+
let msg = errDesc !== '' ? errDesc : err.message?.trim()
290285

291286
if (typeof msg !== 'string') {
292287
return undefined
293288
}
294289

290+
// append the cause's message
291+
if (withCause) {
292+
const errorId = getErrorId(err)
293+
// - prepend id to message
294+
// - If a generic error does not have the `name` field explicitly set, it returns a generic 'Error' name. So skip since it is useless.
295+
if (errorId && errorId !== 'Error') {
296+
msg = `${errorId}: ${msg}`
297+
}
298+
299+
const cause = (err as any).cause
300+
return `${msg}${cause ? ' | ' + getErrorMsg(cause, withCause) : ''}`
301+
}
302+
295303
return msg
296304
}
297305

@@ -420,8 +428,8 @@ export function getTelemetryReasonDesc(err: unknown | undefined): string | undef
420428
const m = typeof err === 'string' ? err : getErrorMsg(err as Error, true) ?? ''
421429
const msg = scrubNames(m, _username)
422430

423-
// Truncate to 200 chars.
424-
return msg && msg.length > 0 ? msg.substring(0, 200) : undefined
431+
// Truncate message as these strings can be very long.
432+
return msg && msg.length > 0 ? msg.substring(0, 350) : undefined
425433
}
426434

427435
export function getTelemetryReason(error: unknown | undefined): string | undefined {
@@ -568,6 +576,16 @@ function hasCode<T>(error: T): error is T & { code: string } {
568576
return typeof (error as { code?: unknown }).code === 'string'
569577
}
570578

579+
/**
580+
* Returns the identifier the given error.
581+
* Depending on the implementation, the identifier may exist on a
582+
* different property.
583+
*/
584+
export function getErrorId(error: Error): string {
585+
// prioritize code over the name
586+
return hasCode(error) ? error.code : error.name
587+
}
588+
571589
function hasTime(error: Error): error is typeof error & { time: Date } {
572590
return (error as { time?: unknown }).time instanceof Date
573591
}

packages/core/src/test/codewhisperer/commands/startSecurityScan.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ describe('startSecurityScan', function () {
385385
codewhispererCodeScanScope: 'PROJECT',
386386
result: 'Failed',
387387
reason: 'CodeScanJobFailedError',
388-
reasonDesc: 'Security scan failed.',
388+
reasonDesc: 'CodeScanJobFailedError: Security scan failed.',
389389
passive: false,
390390
} as unknown as CodewhispererSecurityScan)
391391
})
@@ -413,7 +413,7 @@ describe('startSecurityScan', function () {
413413
codewhispererCodeScanScope: 'PROJECT',
414414
result: 'Failed',
415415
reason: 'ThrottlingException',
416-
reasonDesc: 'Maximum project scan count reached for this month.',
416+
reasonDesc: 'ThrottlingException: Maximum project scan count reached for this month.',
417417
passive: false,
418418
} as unknown as CodewhispererSecurityScan)
419419
})
@@ -444,7 +444,7 @@ describe('startSecurityScan', function () {
444444
codewhispererCodeScanScope: 'FILE',
445445
result: 'Failed',
446446
reason: 'ThrottlingException',
447-
reasonDesc: 'Maximum auto-scans count reached for this month.',
447+
reasonDesc: 'ThrottlingException: Maximum auto-scans count reached for this month.',
448448
passive: true,
449449
} as unknown as CodewhispererSecurityScan)
450450
})

packages/core/src/test/shared/errors.test.ts

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
tryRun,
2121
UnknownError,
2222
DiskCacheError,
23+
getErrorId,
2324
} from '../../shared/errors'
2425
import { CancellationError } from '../../shared/utilities/timeoutUtils'
2526
import { UnauthorizedException } from '@aws-sdk/client-sso'
@@ -447,6 +448,22 @@ describe('util', function () {
447448
)
448449
})
449450

451+
it('getErrorId', function () {
452+
let error = new Error()
453+
assert.deepStrictEqual(getErrorId(error), 'Error')
454+
455+
error = new Error()
456+
error.name = 'MyError'
457+
assert.deepStrictEqual(getErrorId(error), 'MyError')
458+
459+
error = new ToolkitError('', { code: 'MyCode' })
460+
assert.deepStrictEqual(getErrorId(error), 'MyCode')
461+
462+
// `code` takes priority over `name`
463+
error = new ToolkitError('', { code: 'MyCode', name: 'MyError' })
464+
assert.deepStrictEqual(getErrorId(error), 'MyCode')
465+
})
466+
450467
it('getErrorMsg()', function () {
451468
assert.deepStrictEqual(
452469
getErrorMsg(
@@ -465,23 +482,36 @@ describe('util', function () {
465482
assert.deepStrictEqual(getErrorMsg(awsErr), 'aws error desc 1')
466483

467484
// Arg withCause=true
468-
awsErr = new TestAwsError('ValidationException', 'aws validation msg 1', new Date())
469485
let toolkitError = new ToolkitError('ToolkitError Message')
470486
assert.deepStrictEqual(getErrorMsg(toolkitError, true), 'ToolkitError Message')
471487

488+
awsErr = new TestAwsError('ValidationException', 'aws validation msg 1', new Date())
472489
toolkitError = new ToolkitError('ToolkitError Message', { cause: awsErr })
473-
assert.deepStrictEqual(getErrorMsg(toolkitError, true), `ToolkitError Message | aws validation msg 1`)
490+
assert.deepStrictEqual(
491+
getErrorMsg(toolkitError, true),
492+
`ToolkitError Message | ValidationException: aws validation msg 1`
493+
)
474494

475-
const nestedNestedToolkitError = new ToolkitError('C')
476-
const nestedToolkitError = new ToolkitError('B', { cause: nestedNestedToolkitError })
477-
toolkitError = new ToolkitError('A', { cause: nestedToolkitError })
478-
assert.deepStrictEqual(getErrorMsg(toolkitError, true), `A | B | C`)
495+
const nestedNestedToolkitError = new Error('C')
496+
nestedNestedToolkitError.name = 'NameC'
497+
const nestedToolkitError = new ToolkitError('B', { cause: nestedNestedToolkitError, code: 'CodeB' })
498+
toolkitError = new ToolkitError('A', { cause: nestedToolkitError, code: 'CodeA' })
499+
assert.deepStrictEqual(getErrorMsg(toolkitError, true), `CodeA: A | CodeB: B | NameC: C`)
500+
501+
// Arg withCause=true excludes the generic 'Error' id
502+
const errorWithGenericName = new Error('A') // note this does not set a value for `name`, by default it is 'Error'
503+
assert.deepStrictEqual(getErrorMsg(errorWithGenericName, true), `A`)
504+
errorWithGenericName.name = 'NameA' // now we set a `name`
505+
assert.deepStrictEqual(getErrorMsg(errorWithGenericName, true), `NameA: A`)
479506
})
480507

481508
it('getTelemetryReasonDesc()', () => {
482509
const err = new Error('Cause Message a/b/c/d.txt')
483-
const toolkitError = new ToolkitError('ToolkitError Message', { cause: err })
484-
assert.deepStrictEqual(getTelemetryReasonDesc(toolkitError), 'ToolkitError Message | Cause Message x/x/x/x.txt')
510+
const toolkitError = new ToolkitError('ToolkitError Message', { cause: err, code: 'CodeA' })
511+
assert.deepStrictEqual(
512+
getTelemetryReasonDesc(toolkitError),
513+
'CodeA: ToolkitError Message | Cause Message x/x/x/x.txt'
514+
)
485515
})
486516

487517
function makeSyntaxErrorWithSdkClientError() {

packages/core/src/test/shared/telemetry/spans.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ describe('TelemetryTracer', function () {
258258
assertTelemetry('aws_loginWithBrowser', {
259259
result: 'Failed',
260260
reason: 'InvalidRequestException',
261-
reasonDesc: 'Invalid client ID provided',
261+
reasonDesc: 'InvalidRequestException: Invalid client ID provided',
262262
httpStatusCode: '400',
263263
})
264264
const metric = getMetrics('aws_loginWithBrowser')[0]

0 commit comments

Comments
 (0)