-
Notifications
You must be signed in to change notification settings - Fork 29
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
chore(open-payments): adding debug logs for client initialization & requests #485
Changes from all commits
1779e69
1225de3
f04588b
7cbcfe2
8f8f9e0
00d17f0
ada7c15
0080304
7a78eb9
0b90799
a763464
d91c08d
6515675
2a9b27f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
'@interledger/open-payments': minor | ||
--- | ||
|
||
- Adding debug logs during client initialization | ||
- Adding debug logs for requests and responses in the http client | ||
- Making log level `silent` by default |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,7 @@ description: "Sets node version, init pnpm, restore cache" | |
runs: | ||
using: "composite" | ||
steps: | ||
- uses: pnpm/action-setup@v2 | ||
with: | ||
version: 8 | ||
- uses: pnpm/action-setup@v4 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
- uses: actions/setup-node@v3 | ||
with: | ||
node-version: 20 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,7 @@ const incomingPayment = await client.walletAddress.get({ | |
| ------------------- | --------------------------------------------------------------------------------------------------------------- | | ||
| `requestTimeoutMs` | (optional) The timeout in ms for each request by the client. Defaults to 5000. | | ||
| `logger` | (optional) The custom logger to provide for the client. Defaults to pino logger. | | ||
| `logLevel` | (optional) The log level for the client. Defaults to `info`. | | ||
| `logLevel` | (optional) The log level for the client. Defaults to `silent`. | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think external libraries like ours should only log if explicitly specified |
||
| `validateResponses` | (optional) Enables or disables response validation against the Open Payments OpenAPI specs. Defaults to `true`. | | ||
|
||
### `AuthenticatedClient` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,31 +97,31 @@ export interface CollectionRequestArgs | |
walletAddress: string | ||
} | ||
|
||
const parseKey = ( | ||
args: Partial<CreateAuthenticatedClientArgs> | ||
): KeyObject | undefined => { | ||
if (!args.privateKey) { | ||
return undefined | ||
const parseKey = (deps: { logger: Logger }, privateKey: KeyLike): KeyObject => { | ||
if (privateKey instanceof KeyObject) { | ||
deps.logger.debug('Loading key from KeyObject') | ||
return privateKey | ||
} | ||
|
||
if (args.privateKey instanceof KeyObject) { | ||
return args.privateKey | ||
} | ||
|
||
if (args.privateKey instanceof Buffer) { | ||
if (privateKey instanceof Buffer) { | ||
try { | ||
return createPrivateKey(args.privateKey) | ||
deps.logger.debug('Loading key from Buffer') | ||
return createPrivateKey(privateKey) | ||
} catch { | ||
throw new Error('Key is not a valid file') | ||
} | ||
} | ||
|
||
if (fs.existsSync(path.resolve(process.cwd(), args.privateKey))) { | ||
return loadKey(path.resolve(process.cwd(), args.privateKey)) | ||
const keyFilePath = path.resolve(process.cwd(), privateKey) | ||
|
||
if (fs.existsSync(keyFilePath)) { | ||
deps.logger.debug(`Loading key from file path: ${keyFilePath}`) | ||
return loadKey(keyFilePath) | ||
} | ||
|
||
try { | ||
return createPrivateKey(args.privateKey) | ||
deps.logger.debug('Loading key from string') | ||
return createPrivateKey(privateKey) | ||
} catch { | ||
throw new Error('Key is not a valid path or file') | ||
} | ||
|
@@ -132,14 +132,16 @@ const createUnauthenticatedDeps = async ({ | |
validateResponses = true, | ||
...args | ||
}: Partial<CreateUnauthenticatedClientArgs> = {}): Promise<UnauthenticatedClientDeps> => { | ||
const logger = args?.logger ?? createLogger({ name: 'Open Payments Client' }) | ||
const logger = | ||
args.logger ?? | ||
createLogger({ name: 'Open Payments Client', level: 'silent' }) | ||
if (args.logLevel) { | ||
logger.level = args.logLevel | ||
} | ||
|
||
const httpClient = await createHttpClient({ | ||
requestTimeoutMs: | ||
args?.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS | ||
logger, | ||
requestTimeoutMs: args.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS | ||
}) | ||
|
||
let walletAddressServerOpenApi: OpenAPI | undefined | ||
|
@@ -166,40 +168,49 @@ const createAuthenticatedClientDeps = async ({ | |
}: | ||
| CreateAuthenticatedClientArgs | ||
| CreateAuthenticatedClientWithReqInterceptorArgs): Promise<AuthenticatedClientDeps> => { | ||
const logger = args?.logger ?? createLogger({ name: 'Open Payments Client' }) | ||
const logger = | ||
args.logger ?? | ||
createLogger({ name: 'Open Payments Client', level: 'silent' }) | ||
if (args.logLevel) { | ||
logger.level = args.logLevel | ||
} | ||
|
||
let privateKey: KeyObject | undefined | ||
try { | ||
privateKey = parseKey(args) | ||
} catch (error) { | ||
const errorMessage = | ||
'Could not load private key when creating Open Payments client' | ||
const description = error instanceof Error ? error.message : 'Unknown error' | ||
|
||
logger.error({ description }, errorMessage) | ||
|
||
throw new OpenPaymentsClientError(errorMessage, { | ||
description | ||
}) | ||
} | ||
|
||
let httpClient: HttpClient | ||
|
||
if ('authenticatedRequestInterceptor' in args) { | ||
httpClient = await createHttpClient({ | ||
logger, | ||
requestTimeoutMs: | ||
args?.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS, | ||
authenticatedRequestInterceptor: args.authenticatedRequestInterceptor | ||
args.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS, | ||
requestSigningArgs: { | ||
authenticatedRequestInterceptor: args.authenticatedRequestInterceptor | ||
} | ||
}) | ||
} else { | ||
let privateKey: KeyObject | ||
try { | ||
privateKey = parseKey({ logger }, args.privateKey) | ||
} catch (error) { | ||
const errorMessage = | ||
'Could not load private key when creating authenticated client' | ||
const description = | ||
error instanceof Error ? error.message : 'Unknown error' | ||
|
||
logger.error({ description }, errorMessage) | ||
|
||
throw new OpenPaymentsClientError(errorMessage, { | ||
description | ||
}) | ||
Comment on lines
+201
to
+203
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be worthwhile to pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that would be a nice improvement to the errors. I think this is something to tackle when updating Typescript in the repo, as we currently don't have the updated types for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aside: On updating TypeScript, I was updating There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, maybe in a next sprint I can go through it 😅 |
||
} | ||
|
||
httpClient = await createHttpClient({ | ||
logger, | ||
requestTimeoutMs: | ||
args?.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS, | ||
privateKey, | ||
keyId: args.keyId | ||
args.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS, | ||
requestSigningArgs: { | ||
privateKey, | ||
keyId: args.keyId | ||
} | ||
}) | ||
} | ||
|
||
|
@@ -250,6 +261,16 @@ export const createUnauthenticatedClient = async ( | |
const { resourceServerOpenApi, walletAddressServerOpenApi, ...baseDeps } = | ||
await createUnauthenticatedDeps(args) | ||
|
||
baseDeps.logger.debug( | ||
{ | ||
validateResponses: !!args.validateResponses, | ||
useHttp: baseDeps.useHttp, | ||
requestTimeoutMs: | ||
args.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS | ||
}, | ||
'Created unauthenticated client' | ||
) | ||
|
||
return { | ||
walletAddress: createWalletAddressRoutes({ | ||
...baseDeps, | ||
|
@@ -335,6 +356,18 @@ export async function createAuthenticatedClient( | |
...baseDeps | ||
} = await createAuthenticatedClientDeps(args) | ||
|
||
baseDeps.logger.debug( | ||
{ | ||
walletAddressUrl: args.walletAddressUrl, | ||
...('keyId' in args ? { keyId: args.keyId } : {}), | ||
validateResponses: !!args.validateResponses, | ||
useHttp: baseDeps.useHttp, | ||
requestTimeoutMs: | ||
args.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS | ||
}, | ||
'Created authenticated client' | ||
) | ||
|
||
return { | ||
incomingPayment: createIncomingPaymentRoutes({ | ||
...baseDeps, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise: