Skip to content

Commit ec03cab

Browse files
HweinstockHweinstock
committed
refactor: simplify implementation
1 parent 4a9708f commit ec03cab

File tree

5 files changed

+97
-132
lines changed

5 files changed

+97
-132
lines changed

‎packages/core/src/shared/awsClientBuilderV3.ts‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -263,21 +263,27 @@ export async function onDeserialize<Input extends object, Output extends object>
263263
const err = { ...e, name: e.name, mesage: e.message }
264264
delete err['stack']
265265
recordErrorTelemetry(err, serviceId)
266-
logErrorWithHeaders(request, err, logTail)
266+
getLogger().error(`API Error %s: %O`, logTail, err)
267267
}
268268
throw e
269269
}
270270
}
271271

272-
export async function logOnFinalize<Input extends object, Output extends object>(
272+
export function logOnFinalize<Input extends object, Output extends object>(
273273
next: FinalizeHandler<Input, Output>,
274274
args: FinalizeHandlerArguments<Input>
275275
) {
276276
const request = args.request
277277
if (HttpRequest.isInstance(request)) {
278278
const { hostname, path } = request
279279
const input = partialClone(args.input, 3, ['clientSecret', 'accessToken', 'refreshToken'], '[omitted]')
280-
getLogger().debug(`API Request (%s %s): %O`, hostname, path, input)
280+
getLogger().debug(
281+
`API Request (%s %s):\n headers: %O\n input: %O`,
282+
hostname,
283+
path,
284+
filterRequestHeaders(request),
285+
input
286+
)
281287
}
282288
return next(args)
283289
}
@@ -319,7 +325,7 @@ export function addKeepAliveHeader<Input extends object, Output extends object>(
319325
return next(args)
320326
}
321327

322-
export function logErrorWithHeaders(response: HttpRequest, err: Omit<Error, 'stack'>, logTail?: string) {
328+
function filterRequestHeaders(request: HttpRequest) {
323329
const logHeaderNames = [
324330
'x-amzn-requestid',
325331
'x-amzn-trace-id',
@@ -328,8 +334,5 @@ export function logErrorWithHeaders(response: HttpRequest, err: Omit<Error, 'sta
328334
'x-amz-cf-id',
329335
'x-amz-cf-pop',
330336
]
331-
const filteredHeaders = Object.fromEntries(
332-
Object.entries(response.headers).filter(([k, v]) => logHeaderNames.includes(k))
333-
)
334-
getLogger().error('API request failed %s\n headers: %O\n %O', logTail, filteredHeaders, err)
337+
return Object.fromEntries(Object.entries(request.headers).filter(([k, _]) => logHeaderNames.includes(k)))
335338
}

‎packages/core/src/shared/clients/clientWrapper.ts‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,11 @@ import globals from '../extensionGlobals'
77
import { AwsClient, AwsClientConstructor, AwsCommand, AwsCommandConstructor } from '../awsClientBuilderV3'
88
import { PaginationConfiguration, Paginator } from '@aws-sdk/types'
99
import { AsyncCollection, toCollection } from '../utilities/asyncCollection'
10-
import { isDefined } from '../utilities/tsUtils'
11-
import { withPerfLogOnFail } from '../logger/perfLogger'
10+
import { hasKey, isDefined } from '../utilities/tsUtils'
11+
import { PerfLog } from '../logger/perfLogger'
1212
import { truncateProps } from '../utilities/textUtilities'
13+
import { getLogger } from '../logger/logger'
14+
import { ToolkitError } from '../errors'
1315

1416
type SDKPaginator<C, CommandInput extends object, CommandOutput extends object> = (
1517
config: Omit<PaginationConfiguration, 'client'> & { client: C },
@@ -37,13 +39,25 @@ export abstract class ClientWrapper<C extends AwsClient> implements vscode.Dispo
3739
CommandOptions extends CommandInput,
3840
Command extends AwsCommand<CommandInput, CommandOutput>,
3941
>(command: AwsCommandConstructor<CommandInput, Command>, commandOptions: CommandOptions): Promise<CommandOutput> {
40-
const makeRequest = withPerfLogOnFail(
41-
'API Request',
42-
() => this.getClient().send(new command(commandOptions)),
43-
truncateProps(commandOptions, 20, ['nextToken'])
44-
)
45-
46-
return makeRequest()
42+
const action = 'API Request'
43+
const perflog = new PerfLog(action)
44+
return await this.getClient()
45+
.send(new command(commandOptions))
46+
.catch((e: Error) => {
47+
const errWithoutStack = { ...e, name: e.name, message: e.message }
48+
delete errWithoutStack['stack']
49+
const timecost = perflog.elapsed().toFixed(1)
50+
getLogger().error(
51+
`${action} failed (time: %dms) \nparams: %O\nerror: %O`,
52+
timecost,
53+
truncateProps(commandOptions, 20, ['nextToken']),
54+
errWithoutStack
55+
)
56+
throw new ToolkitError(`${action}: ${errWithoutStack.message}`, {
57+
code: extractCode(errWithoutStack),
58+
cause: errWithoutStack,
59+
})
60+
})
4761
}
4862

4963
protected makePaginatedRequest<CommandInput extends object, CommandOutput extends object, Output extends object>(
@@ -73,3 +87,11 @@ export abstract class ClientWrapper<C extends AwsClient> implements vscode.Dispo
7387
this.client?.destroy()
7488
}
7589
}
90+
91+
function extractCode(e: Error): string {
92+
return hasKey(e, 'code') && typeof e['code'] === 'string'
93+
? e.code
94+
: hasKey(e, 'Code') && typeof e['Code'] === 'string'
95+
? e.Code
96+
: e.name
97+
}

‎packages/core/src/shared/logger/perfLogger.ts‎

Lines changed: 50 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
import { ToolkitError } from '../errors'
76
import { getLogger } from './logger'
87

98
export class PerfLog {
@@ -29,53 +28,53 @@ export class PerfLog {
2928
}
3029
}
3130

32-
/**
33-
* Call a function f and if it fails, log the error with performance information included.
34-
* @param action label of action to include in the error log.
35-
* @param f action to attempt.
36-
* @param params params that were passed to f. Defaults to empty.
37-
* @param getCode optional mapping to extract code from error. Defaults to name of error.
38-
* @returns result of f
39-
*/
40-
export function withPerfLogOnFail<Result>(
41-
action: string,
42-
f: () => Result,
43-
params?: object,
44-
getCode?: (e: Error) => string
45-
): () => Result
46-
export function withPerfLogOnFail<Result>(
47-
action: string,
48-
f: () => Promise<Result>,
49-
params?: object,
50-
getCode?: (e: Error) => string
51-
): () => Promise<Result>
52-
export function withPerfLogOnFail<Result>(
53-
action: string,
54-
f: () => Result | Promise<Result>,
55-
params?: object,
56-
getCode?: (e: Error) => string
57-
) {
58-
return function () {
59-
const perflog = new PerfLog(action)
60-
try {
61-
return f()
62-
} catch (e) {
63-
if (e instanceof Error) {
64-
const errWithoutStack = { ...e, name: e.name, message: e.message }
65-
delete errWithoutStack['stack']
66-
const timecost = perflog.elapsed().toFixed(1)
67-
getLogger().error(
68-
`${action} failed (time: %dms) \nparams: %O\nerror: %O`,
69-
timecost,
70-
params ?? {},
71-
errWithoutStack
72-
)
73-
throw new ToolkitError(`${action}: ${errWithoutStack.message}`, {
74-
code: getCode ? getCode(errWithoutStack) : errWithoutStack.name,
75-
cause: errWithoutStack,
76-
})
77-
}
78-
throw e
79-
}
80-
}
81-
}
31+
// /**
32+
// * Call a function f and if it fails, log the error with performance information included.
33+
// * @param action label of action to include in the error log.
34+
// * @param f action to attempt.
35+
// * @param params params that were passed to f. Defaults to empty.
36+
// * @param getCode optional mapping to extract code from error. Defaults to name of error.
37+
// * @returns result of f
38+
// */
39+
// export function withPerfLogOnFail<Result>(
40+
// action: string,
41+
// f: () => Result,
42+
// params?: object,
43+
// getCode?: (e: Error) => string
44+
// ): () => Result
45+
// export function withPerfLogOnFail<Result>(
46+
// action: string,
47+
// f: () => Promise<Result>,
48+
// params?: object,
49+
// getCode?: (e: Error) => string
50+
// ): () => Promise<Result>
51+
// export function withPerfLogOnFail<Result>(
52+
// action: string,
53+
// f: () => Result | Promise<Result>,
54+
// params?: object,
55+
// getCode?: (e: Error) => string
56+
// ) {
57+
// return function () {
58+
// const perflog = new PerfLog(action)
59+
// try {
60+
// return f()
61+
// } catch (e) {
62+
// if (e instanceof Error) {
63+
// const errWithoutStack = { ...e, name: e.name, message: e.message }
64+
// delete errWithoutStack['stack']
65+
// const timecost = perflog.elapsed().toFixed(1)
66+
// getLogger().error(
67+
// `${action} failed (time: %dms) \nparams: %O\nerror: %O`,
68+
// timecost,
69+
// params ?? {},
70+
// errWithoutStack
71+
// )
72+
// throw new ToolkitError(`${action}: ${errWithoutStack.message}`, {
73+
// code: getCode ? getCode(errWithoutStack) : errWithoutStack.name,
74+
// cause: errWithoutStack,
75+
// })
76+
// }
77+
// throw e
78+
// }
79+
// }
80+
// }

‎packages/core/src/test/shared/awsClientBuilderV3.test.ts‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import {
1919
logOnFinalize,
2020
overwriteEndpoint,
2121
recordErrorTelemetry,
22-
logErrorWithHeaders,
2322
} from '../../shared/awsClientBuilderV3'
2423
import { Client } from '@aws-sdk/smithy-client'
2524
import { DevSettings, extensionVersion } from '../../shared'
@@ -281,20 +280,12 @@ describe('AwsClientBuilderV3', function () {
281280
assert.strictEqual(newArgs.request.path, '/testPath')
282281
})
283282

284-
it('logs specific headers on request failure', async function () {
285-
const response = new HttpRequest({
286-
body: 'test body',
287-
headers: {
288-
'x-amzn-RequestId': 'fakeId',
289-
'x-amzn-requestid': 'realId',
290-
'x-amz-id-2': 'otherId',
291-
},
292-
})
283+
it('logs specific headers in the filter list', async function () {
284+
const next = async (args: any) => args
285+
await logOnFinalize(next, args)
293286

294-
logErrorWithHeaders(response, new Error('test error'), 'host-path')
295-
assertLogsContain('realId', false, 'error')
296-
assert.throws(() => assertLogsContain('fakeId', false, 'error'))
297-
assert.throws(() => assertLogsContain('otherId', false, 'error'))
287+
assertLogsContain('realId', false, 'debug')
288+
assert.throws(() => assertLogsContain('fakeId', false, 'debug'))
298289
})
299290
})
300291

‎packages/core/src/test/shared/logger/perflog.test.ts‎

Lines changed: 0 additions & 50 deletions
This file was deleted.

0 commit comments

Comments
 (0)