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
Expand Up @@ -29,9 +29,11 @@ export function createAccountMiddleware(serverAddress: string): ILPMiddleware {
)
} 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
}
}
Expand All @@ -56,7 +58,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 disabled'
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe "destination account is in an incorrect state" makes more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya thats better, updated

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 +77,7 @@ export function createAccountMiddleware(serverAddress: string): ILPMiddleware {
LiquidityAccountType.INCOMING
)
}
ctx.services.logger.trace('got account from incoming payment')
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe "destination account is incoming payment"? maybe we can also log the id of the account?

Copy link
Contributor Author

@BlairCurrey BlairCurrey May 1, 2024

Choose a reason for hiding this comment

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

I'm interpreting "id of the account" to mean incomingPayment in this case. As opposed to the created liquidity account (not sure if that id would be the same or not). Correct me if needed.

I updated this and similar to log this message and the underlying account id (in this case incoming payment).

return incomingPayment
}
// Open Payments SPSP fallback account
Expand All @@ -80,12 +91,14 @@ export function createAccountMiddleware(serverAddress: string): ILPMiddleware {
LiquidityAccountType.WEB_MONETIZATION
)
}
ctx.services.logger.trace('got account from wallet address')
return walletAddress
}
}
const address = ctx.request.prepare.destination
const peer = await peers.getByDestinationAddress(address)
if (peer) {
ctx.services.logger.trace('got account from peer')
return peer
}
if (
Expand All @@ -106,7 +119,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
@@ -1,5 +1,5 @@
import { Errors } from 'ilp-packet'
import { ILPContext, ILPMiddleware } from '../rafiki'
import { ILPContext, ILPMiddleware, TransferOptions } from '../rafiki'
BlairCurrey marked this conversation as resolved.
Show resolved Hide resolved
import {
isTransferError,
TransferError
Expand Down Expand Up @@ -35,6 +35,15 @@ export function createBalanceMiddleware(): ILPMiddleware {
})
if (typeof destinationAmountOrError !== 'bigint') {
// ConvertError
services.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,20 +60,35 @@ 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)) {
const safeLogTransferError = (transferOptions: TransferOptions) => {
if (transferOptions.destinationAccount?.http?.outgoing.authToken) {
// @ts-expect-error - "The operand of a 'delete' operator must be optional"
delete transferOptions.destinationAccount.http.outgoing.authToken
}
services.logger.error(
{ transferOptions },
'Could not create transfer'
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also log the trxOrError here?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want, you can also use redaction in pino on a global level (to avoid needing to delete keys and such)

Copy link
Contributor Author

@BlairCurrey BlairCurrey May 1, 2024

Choose a reason for hiding this comment

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

I will try to use that instead - looks handy. I found this surprisingly cumbersome. Although I dont love the delete with @ts-expect-error it seemed like the best we could do so I'm glad to learn about that feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to log the trxOrError and used redact instead

)
}

switch (trxOrError) {
case TransferError.InsufficientBalance:
case TransferError.InsufficientLiquidity:
safeLogTransferError(transferOptions)
throw new InsufficientLiquidityError(trxOrError)
default:
safeLogTransferError(transferOptions)
// TODO: map transfer errors to ILP errors
ctxThrow(500, destinationAmountOrError.toString())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ 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()
}
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 @@ -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
Loading