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

feat(backend): improve ilp connector logs #2680

Merged
merged 11 commits into from
May 7, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,12 @@ export function createStreamController(): ILPMiddleware {
} else {
logger.warn(
{
connectionKey,
totalReceived,
err,
err2
},
'error incrementing stream totalReceived'
'failed to increment stream totalReceived'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no error actually being throw here, so I changed the wording

)
}
response.reply = moneyOrReply.accept()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import { Factory } from 'rosie'
import { Factory, IFactory } from 'rosie'
import { Logger } from 'pino'

export const TestLoggerFactory = Factory.define<Logger>('TestLogger').attrs({
export const TestLoggerFactory: IFactory<Logger> = Factory.define<Logger>(
'TestLogger'
).attrs({
debug: () => jest.fn(),
fatal: () => jest.fn(),
error: () => jest.fn(),
warn: () => jest.fn(),
info: () => jest.fn(),
trace: () => jest.fn()
trace: () => jest.fn(),
child: () => jest.fn().mockImplementation(() => TestLoggerFactory.build())
})
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,31 @@ export function createAccountMiddleware(serverAddress: string): ILPMiddleware {
account,
accountType
)
ctx.services.logger.debug(
{ account, accountType },
'Created liquidity account'
)
} catch (err) {
// Don't complain if liquidity account already exists.
if (err instanceof AccountAlreadyExistsError) {
// Do nothing.
} else {
if (!(err instanceof AccountAlreadyExistsError)) {
Copy link
Contributor

@njlie njlie May 3, 2024

Choose a reason for hiding this comment

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

I can't comment on the exact line where this could be added, but in the spirit of the directive in #2489 for "more is more", might it be useful for a debug or trace-level log when liquidity accounts are successfully created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, added: 5ab763c

ctx.services.logger.error(
{ account, accountType, err },
'Failed to create liquidity account'
)
throw err
}
}
}

const { walletAddresses, incomingPayments, peers } = ctx.services
const incomingAccount = ctx.state.incomingAccount
if (!incomingAccount) ctx.throw(401, 'unauthorized')
if (!incomingAccount) {
ctx.services.logger.error(
{ state: ctx.state },
'Unauthorized: No incoming account'
)
ctx.throw(401, 'unauthorized')
}

const getAccountByDestinationAddress = async (): Promise<
OutgoingAccount | undefined
Expand All @@ -56,7 +68,15 @@ export function createAccountMiddleware(serverAddress: string): ILPMiddleware {
IncomingPaymentState.Expired
].includes(incomingPayment.state)
) {
throw new Errors.UnreachableError('destination account is disabled')
const errorMessage = 'destination account is in an incorrect state'
ctx.services.logger.error(
{
incomingPayment,
streamDestination: ctx.state.streamDestination
},
errorMessage
)
throw new Errors.UnreachableError(errorMessage)
}

// Create the tigerbeetle account if not exists.
Expand All @@ -67,6 +87,10 @@ export function createAccountMiddleware(serverAddress: string): ILPMiddleware {
LiquidityAccountType.INCOMING
)
}
ctx.services.logger.debug(
{ incomingPaymentId: incomingPayment.id },
'destination account is incoming payment'
)
return incomingPayment
}
// Open Payments SPSP fallback account
Expand All @@ -80,12 +104,20 @@ export function createAccountMiddleware(serverAddress: string): ILPMiddleware {
LiquidityAccountType.WEB_MONETIZATION
)
}
ctx.services.logger.debug(
{ walletAddressId: walletAddress.id },
'destination account is wallet address'
)
return walletAddress
}
}
const address = ctx.request.prepare.destination
const peer = await peers.getByDestinationAddress(address)
if (peer) {
ctx.services.logger.debug(
{ peerId: peer.id },
'destination account is peer'
)
return peer
}
if (
Expand All @@ -106,7 +138,15 @@ export function createAccountMiddleware(serverAddress: string): ILPMiddleware {

const outgoingAccount = await getAccountByDestinationAddress()
if (!outgoingAccount) {
throw new Errors.UnreachableError('unknown destination account')
const errorMessage = 'unknown destination account'
ctx.services.logger.error(
{
streamDestination: ctx.state.streamDestination,
destinationAddress: ctx.request.prepare.destination
},
errorMessage
)
throw new Errors.UnreachableError(errorMessage)
}
ctx.accounts = {
get incoming(): IncomingAccount {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can have a log if we fail to get the incomingAccount as well (on L44)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that makes sense, missed that

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ export function createBalanceMiddleware(): ILPMiddleware {
next: () => Promise<void>
): Promise<void> => {
const { amount } = request.prepare
const logger = services.logger.child(
{ module: 'balance-middleware' },
{
redact: ['transferOptions.destinationAccount.http.outgoing.authToken']
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to just do *token? Probably no reason to log any token related stuff?

Copy link
Contributor Author

@BlairCurrey BlairCurrey May 2, 2024

Choose a reason for hiding this comment

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

I would say yes but the wildcard *token doesnt work like that according to the docs. https://github.com/davidmarkclements/fast-redact?tab=readme-ov-file#wildcards Also getting an error when trying it.

It doesn't match token, authToken, etc. nor would it look for tokens at all depths.

From what I can tell there isn't a way to configure redact to redact all values for a key matching *token at any depth, nor even a key matching *token at a specific depth.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, I'm guessing recursive redacting is quite costly. In that case, maybe we just do what you had previously, or instead of deleting keys how you had before, we can just selectively log a few properties of this object. Sorry for the back-and-forth, thought this was recursive from the beginning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we talked off github, it is probably easier to blacklist sensitive info than whitelist useful info here because there is a fair amount and not exactly sure what will matter. So it currently redacts just the authToken.

}
)

// Ignore zero amount packets
if (amount === '0') {
Expand All @@ -35,6 +41,15 @@ export function createBalanceMiddleware(): ILPMiddleware {
})
if (typeof destinationAmountOrError !== 'bigint') {
// ConvertError
logger.error(
{
amount,
destinationAmountOrError,
sourceAsset: accounts.incoming.asset,
destinationAsset: accounts.outgoing.asset
},
'Could not get rates'
)
throw new CannotReceiveError(
`Exchange rate error: ${destinationAmountOrError}`
)
Expand All @@ -51,15 +66,20 @@ export function createBalanceMiddleware(): ILPMiddleware {
const createPendingTransfer = async (): Promise<
Transaction | undefined
> => {
const trxOrError = await services.accounting.createTransfer({
const transferOptions = {
sourceAccount: accounts.incoming,
destinationAccount: accounts.outgoing,
sourceAmount,
destinationAmount: destinationAmountOrError,
timeout: 5
})

}
const trxOrError =
await services.accounting.createTransfer(transferOptions)
if (isTransferError(trxOrError)) {
logger.error(
{ transferOptions, transferError: trxOrError },
'Could not create transfer'
)
switch (trxOrError) {
case TransferError.InsufficientBalance:
case TransferError.InsufficientLiquidity:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { errorToIlpReject, isIlpError, IlpErrorCode } from 'ilp-packet'
import { HttpError } from 'koa'
import { ILPContext, ILPMiddleware } from '../rafiki'

/**
Expand All @@ -23,16 +22,17 @@ export function createIncomingErrorHandlerMiddleware(
if (!err || typeof err !== 'object') {
err = new Error('Non-object thrown: ' + e)
}
ctx.services.logger.debug({ err }, 'Error thrown in incoming pipeline')
ctx.services.logger.info({ err }, 'Error thrown in incoming pipeline')
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 figured we don't want to limit the error logs to development so I changed to info (which matches what we do in the open payments server error handler as well.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should have an error log in the unexpected internal error case down below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is one but it looks like its basically null if its not an HttpError. changed it to always log with the original error.

if (ctx.revertTotalReceived) {
await ctx.revertTotalReceived()
}
if (isIlpError(err)) {
ctx.response.reject = errorToIlpReject(serverAddress, err)
} else {
ctx.services.logger.error(err instanceof HttpError && err.message)
const message = 'unexpected internal error'
ctx.services.logger.error({ err }, message)
ctx.response.reject = errorToIlpReject(serverAddress, {
message: 'unexpected internal error',
message,
ilpErrorCode: IlpErrorCode.T00_INTERNAL_ERROR,
name: ''
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function createIldcpMiddleware(serverAddress: string): ILPMiddleware {

const clientAddress = incoming.staticIlpAddress
if (!clientAddress) {
logger.warn(
logger.error(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For logging where there are errors, I generally made them error level like this. There were a few exceptions like the rate limiting errors. Those are currently warnings and I left them alone - warning felt appropriate there.

{
peerId: incoming.id
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function createIncomingMaxPacketAmountMiddleware(): ILPMiddleware {
if (maxPacketAmount) {
const amount = request.prepare.intAmount
if (amount > maxPacketAmount) {
logger.warn(
logger.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

I was back and forth on this one, but I think error is correct here, since it would probably require actioning to update the configuration for the peer

{ maxPacketAmount: maxPacketAmount.toString(), request },
'rejected a packet due to amount exceeding maxPacketAmount'
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function getDestinationExpiry(
const sourceExpiryTime = request.expiresAt.getTime()

if (sourceExpiryTime < Date.now()) {
log.trace('incoming packet has already expired', { request })
log.debug({ request }, 'incoming packet has already expired')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pino doesnt complain, but {request} did not end up getting logged when put in the original order.

throw new InsufficientTimeoutError(
'source transfer has already expired.' +
' sourceExpiry=' +
Expand All @@ -42,9 +42,15 @@ function getDestinationExpiry(
)

if (destinationExpiryTime < Date.now() + minExpirationWindow) {
log.trace('incoming packet expires too soon to complete payment', {
request
})
log.debug(
{
expectedDestinationExpiryTime,
maxHoldTime,
request,
minExpirationWindow
},
'incoming packet expires too soon to complete payment'
)
throw new InsufficientTimeoutError(
'source transfer expires too soon to complete payment.' +
' actualSourceExpiry=' +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ export function createOutgoingValidateFulfillmentMiddleware(): ILPMiddleware {
.update(fulfillment)
.digest()
if (!calculatedCondition.equals(executionCondition)) {
logger.warn({ response }, 'invalid fulfillment')
logger.error(
{ response, calculatedCondition, executionCondition },
'invalid fulfillment'
)
throw new WrongConditionError(
'fulfillment did not match expected value.'
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ describe('Account Middleware', () => {
expect(ctx.accounts.outgoing).toEqual(outgoingAccount)
})

test('return an error when the destination account is disabled', async () => {
test('return an error when the destination account is in an incorrect state', async () => {
const outgoingAccount = IncomingPaymentAccountFactory.build({
id: 'deactivatedIncomingPayment',
state: 'COMPLETED'
Expand All @@ -122,7 +122,7 @@ describe('Account Middleware', () => {
}
})
await expect(middleware(ctx, next)).rejects.toThrow(
'destination account is disabled'
'destination account is in an incorrect state'
)
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('Error Handler Middleware', () => {
await expect(middleware(ctx, next)).resolves.toBeUndefined()

expect(ctx.response.reject).toBeDefined()
expect(ctx.services.logger.debug).toHaveBeenCalledWith(
expect(ctx.services.logger.info).toHaveBeenCalledWith(
{
err: errorToBeThrown
},
Expand Down Expand Up @@ -50,7 +50,7 @@ describe('Error Handler Middleware', () => {
expect(ctx.response.reject).toEqual(
errorToIlpReject(ADDRESS, errorToBeThrown)
)
expect(ctx.services.logger.debug).toHaveBeenCalledWith(
expect(ctx.services.logger.info).toHaveBeenCalledWith(
{
err: errorToBeThrown
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('Max Packet Amount Middleware', function () {
expect(next).toHaveBeenCalled()
})

test('throws AmounTooLargeError and logs warning if prepare amount exceeds maximum balance for the peer', async () => {
test('throws AmounTooLargeError and logs error if prepare amount exceeds maximum balance for the peer', async () => {
const prepareOverMaxAmount = IlpPrepareFactory.build({ amount: '51' })
ctx.request.prepare = new ZeroCopyIlpPrepare(prepareOverMaxAmount)
const next = jest.fn()
Expand All @@ -61,7 +61,7 @@ describe('Max Packet Amount Middleware', function () {
)

expect(next).toHaveBeenCalledTimes(0)
expect(ctx.services.logger.warn).toHaveBeenCalledWith(
expect(ctx.services.logger.error).toHaveBeenCalledWith(
{
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
maxPacketAmount: alice.maxPacketAmount!.toString(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('Validate Fulfillment Middleware', function () {
await expect(middleware(ctx, next)).rejects.toBeInstanceOf(
WrongConditionError
)
expect(ctx.services.logger.warn).toHaveBeenCalled()
expect(ctx.services.logger.error).toHaveBeenCalled()
})

test('forwards fulfills with correct fullfillment', async () => {
Expand Down
Loading