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

chore(open-payments): adding debug logs for client initialization & requests #485

Merged
merged 14 commits into from
Jul 4, 2024
Merged
Show file tree
Hide file tree
Changes from 12 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
7 changes: 7 additions & 0 deletions .changeset/brave-pots-rest.md
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
2 changes: 1 addition & 1 deletion packages/open-payments/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`. |
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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`
Expand Down
4 changes: 2 additions & 2 deletions packages/open-payments/src/client/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ describe('Client', (): void => {
} catch (error) {
assert.ok(error instanceof OpenPaymentsClientError)
expect(error.message).toBe(
'Could not load private key when creating Open Payments client'
'Could not load private key when creating authenticated client'
)
expect(error.description).toBe('Key is not a valid file')
}
Expand All @@ -104,7 +104,7 @@ describe('Client', (): void => {
} catch (error) {
assert.ok(error instanceof OpenPaymentsClientError)
expect(error.message).toBe(
'Could not load private key when creating Open Payments client'
'Could not load private key when creating authenticated client'
)
expect(error.description).toBe('Key is not a valid path or file')
}
Expand Down
102 changes: 68 additions & 34 deletions packages/open-payments/src/client/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
}
Expand All @@ -132,12 +132,15 @@ 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({
logger,
requestTimeoutMs:
args?.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS
})
Expand Down Expand Up @@ -166,40 +169,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
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
Copy link
Member

Choose a reason for hiding this comment

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

Might be worthwhile to pass Error.cause for better debugging sometime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Error class that includes cause. We can do it then such that we do it properly instead of using ts-ignore

Copy link
Member

@sidvishnoi sidvishnoi Jul 4, 2024

Choose a reason for hiding this comment

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

Aside: On updating TypeScript, I was updating @apidevtools/json-schema-ref-parser today (due to some node-fetch error it apparently causes on Windows), but then it required updating pretty much all dependencies, from openapi-typescript to TypeScript to eslint 😄.
Should do that sometime - nearly all dependencies pretty out of date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should do that sometime - nearly all dependencies pretty out of date

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
requestSigningArgs: {
privateKey,
keyId: args.keyId
}
})
}

Expand Down Expand Up @@ -250,6 +262,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
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
},
'Created unauthenticated client'
)

return {
walletAddress: createWalletAddressRoutes({
...baseDeps,
Expand Down Expand Up @@ -335,6 +357,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
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
},
'Created authenticated client'
)

return {
incomingPayment: createIncomingPaymentRoutes({
...baseDeps,
Expand Down
76 changes: 59 additions & 17 deletions packages/open-payments/src/client/requests.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ import {
} from './requests'
import { generateKeyPairSync } from 'crypto'
import nock from 'nock'
import { createTestDeps, mockOpenApiResponseValidators } from '../test/helpers'
import {
createTestDeps,
mockOpenApiResponseValidators,
silentLogger
} from '../test/helpers'
import { OpenPaymentsClientError } from './error'
import assert from 'assert'
import ky, { HTTPError } from 'ky'
Expand Down Expand Up @@ -39,9 +43,12 @@ describe('requests', (): void => {

beforeAll(async () => {
httpClient = await createHttpClient({
logger: silentLogger,
requestTimeoutMs: 1000000,
privateKey,
keyId
requestSigningArgs: {
privateKey,
keyId
}
})
deps = await createTestDeps({
httpClient
Expand All @@ -52,7 +59,14 @@ describe('requests', (): void => {
test('sets timeout properly', async (): Promise<void> => {
const kyCreateSpy = jest.spyOn(ky, 'create')

await createHttpClient({ requestTimeoutMs: 1000, privateKey, keyId })
await createHttpClient({
logger: silentLogger,
requestTimeoutMs: 1000,
requestSigningArgs: {
privateKey,
keyId
}
})

expect(kyCreateSpy).toHaveBeenCalledWith(
expect.objectContaining({ timeout: 1000 })
Expand All @@ -61,7 +75,14 @@ describe('requests', (): void => {
test('sets headers properly', async (): Promise<void> => {
const kyCreateSpy = jest.spyOn(ky, 'create')

await createHttpClient({ requestTimeoutMs: 1000, privateKey, keyId })
await createHttpClient({
logger: silentLogger,
requestTimeoutMs: 1000,
requestSigningArgs: {
privateKey,
keyId
}
})

expect(kyCreateSpy).toHaveBeenCalledWith(
expect.objectContaining({
Expand All @@ -81,15 +102,24 @@ describe('requests', (): void => {
const kyExtendSpy = jest.spyOn(kyInstance, 'extend')

await createHttpClient({
logger: silentLogger,
requestTimeoutMs: 0,
authenticatedRequestInterceptor: (config) => config
})

expect(kyExtendSpy).toHaveBeenCalledWith({
hooks: {
beforeRequest: [expect.any(Function)]
requestSigningArgs: {
privateKey,
keyId
}
})

assert.ok(kyExtendSpy.mock.calls[0][0].hooks?.beforeRequest)
expect(kyExtendSpy.mock.calls[0][0].hooks?.beforeRequest).toHaveLength(2)

const requestSignerWithPrivateKey =
kyExtendSpy.mock.calls[0][0].hooks.beforeRequest[1]

const request = new Request('http://localhost:1000')

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
expect(requestSignerWithPrivateKey(request, undefined!)).toBeDefined()
})

test('sets authenticated request interceptor', async (): Promise<void> => {
Expand All @@ -99,16 +129,28 @@ describe('requests', (): void => {

const kyExtendSpy = jest.spyOn(kyInstance, 'extend')

const mockedAuthenticatedRequestInterceptor = jest.fn()

await createHttpClient({
logger: silentLogger,
requestTimeoutMs: 0,
authenticatedRequestInterceptor: (config) => config
})

expect(kyExtendSpy).toHaveBeenCalledWith({
hooks: {
beforeRequest: [expect.any(Function)]
requestSigningArgs: {
authenticatedRequestInterceptor: mockedAuthenticatedRequestInterceptor
}
})

assert.ok(kyExtendSpy.mock.calls[0][0].hooks?.beforeRequest)
expect(kyExtendSpy.mock.calls[0][0].hooks?.beforeRequest).toHaveLength(2)

const authenticatedRequestInterceptor =
kyExtendSpy.mock.calls[0][0].hooks.beforeRequest[1]

const request = new Request('http://localhost:1000', { method: 'POST' })

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
authenticatedRequestInterceptor(request, undefined!)

expect(mockedAuthenticatedRequestInterceptor).toHaveBeenCalled()
})
})

Expand Down
Loading
Loading