Skip to content

Commit

Permalink
feat(2737): add fees as metric for outgoing payment. (#2831)
Browse files Browse the repository at this point in the history 
* feat(2737): add fees as metric for outgoing payment.

* feat(2737): rename to payment_fees.

* feat(2737): test case updates.

* feat(2737): formatting.

* feat(2737): re-order test cases. Move fee collector.

* feat(2737): dashboard and doc updates.

* feat(2737): merged with main.

* feat(2737): review feedback applied from @JoblersTune.

* feat(2737): review feedback applied from @mkurapov.

* feat(2737): additional tests for covert of assets and rates.

* feat(2737): additional tests ensuring the increment counter was called.

* feat(2737): additional tests ensuring the increment counter was called.

* feat(2737): readme.
  • Loading branch information
@koekiebox
koekiebox authored Aug 14, 2024
1 parent 5e81fc1 commit 7a28fd1
Show file tree
Hide file tree
Showing 7 changed files with 316 additions and 14 deletions.
26 changes: 19 additions & 7 deletions packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import { ServiceDependencies } from './service'
import { Receiver } from '../../receiver/model'
import { TransactionOrKnex } from 'objection'
import { ValueType } from '@opentelemetry/api'

// "payment" is locked by the "deps.knex" transaction.
export async function handleSending(
Expand Down Expand Up @@ -85,14 +86,24 @@ export async function handleSending(
const payEndTime = Date.now()

if (deps.telemetry) {
deps.telemetry.incrementCounter('transactions_total', 1, {
description: 'Count of funded transactions'
})

const payDuration = payEndTime - payStartTime
deps.telemetry.recordHistogram('ilp_pay_time_ms', payDuration, {
description: 'Time to complete an ILP payment'
})
await Promise.all([
deps.telemetry.incrementCounter('transactions_total', 1, {
description: 'Count of funded transactions'
}),
deps.telemetry.recordHistogram('ilp_pay_time_ms', payDuration, {
description: 'Time to complete an ILP payment'
}),
deps.telemetry.incrementCounterWithTransactionAmountDifference(
'transaction_fee_amounts',
payment.sentAmount,
payment.receiveAmount,
{
description: 'Amount sent through the network as fees',
valueType: ValueType.DOUBLE
}
)
])
}

await handleCompleted(deps, payment)
Expand Down Expand Up @@ -147,6 +158,7 @@ async function handleCompleted(
await payment.$query(deps.knex).patch({
state: OutgoingPaymentState.Completed
})

await sendWebhookEvent(
deps,
payment,
Expand Down
58 changes: 57 additions & 1 deletion packages/backend/src/open_payments/payment/outgoing/service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import { mockRatesApi } from '../../../tests/rates'
import { UnionOmit } from '../../../shared/utils'
import { QuoteError } from '../../quote/errors'
import { withConfigOverride } from '../../../tests/helpers'
import { TelemetryService } from '../../../telemetry/service'

describe('OutgoingPaymentService', (): void => {
let deps: IocContract<AppServices>
Expand All @@ -58,6 +59,7 @@ describe('OutgoingPaymentService', (): void => {
let accountingService: AccountingService
let paymentMethodHandlerService: PaymentMethodHandlerService
let quoteService: QuoteService
let telemetryService: TelemetryService
let knex: Knex
let walletAddressId: string
let incomingPayment: IncomingPayment
Expand Down Expand Up @@ -243,12 +245,17 @@ describe('OutgoingPaymentService', (): void => {
XRP: exchangeRate
}))

deps = await initIocContainer({ ...Config, exchangeRatesUrl })
deps = await initIocContainer({
...Config,
exchangeRatesUrl,
enableTelemetry: true
})
appContainer = await createTestApp(deps)
outgoingPaymentService = await deps.use('outgoingPaymentService')
accountingService = await deps.use('accountingService')
paymentMethodHandlerService = await deps.use('paymentMethodHandlerService')
quoteService = await deps.use('quoteService')
telemetryService = (await deps.use('telemetry'))!
config = await deps.use('config')
knex = appContainer.knex
})
Expand Down Expand Up @@ -1154,11 +1161,58 @@ describe('OutgoingPaymentService', (): void => {
return payment
}

test('COMPLETED with Telemetry Fee Counter (receiveAmount < incomingPayment.incomingAmount)', async (): Promise<void> => {
const spyTelFeeAmount = jest.spyOn(
telemetryService,
'incrementCounterWithTransactionAmountDifference'
)
const spyCounter = jest.spyOn(telemetryService, 'incrementCounter')

incomingPayment = await createIncomingPayment(deps, {
walletAddressId: receiverWalletAddress.id,
incomingAmount: {
value: receiveAmount.value * 2n,
assetCode: receiverWalletAddress.asset.code,
assetScale: receiverWalletAddress.asset.scale
}
})
assert.ok(incomingPayment.walletAddress)

const createdPayment = await setup({
receiver: incomingPayment.getUrl(incomingPayment.walletAddress),
receiveAmount,
method: 'ilp'
})

mockPaymentHandlerPay(createdPayment, incomingPayment)
const payment = await processNext(
createdPayment.id,
OutgoingPaymentState.Completed
)
await expectOutcome(payment, {
accountBalance: 0n,
amountSent: payment.debitAmount.value,
amountDelivered: payment.receiveAmount.value,
incomingPaymentReceived: payment.receiveAmount.value,
withdrawAmount: 0n
})
// [incrementCounterWithTransactionAmountDifference] called and [incrementCounter] only once due to [Count of funded transactions]
expect(spyTelFeeAmount).toHaveBeenCalledTimes(1)
expect(spyCounter).toHaveBeenCalledTimes(1)
expect(spyCounter).toHaveBeenCalledTimes(1)
})

test.each`
debitAmount | receiveAmount
${debitAmount} | ${undefined}
${undefined} | ${receiveAmount}
`('COMPLETED', async ({ debitAmount, receiveAmount }): Promise<void> => {
const spyTelFeeAmount = jest.spyOn(
telemetryService,
'incrementCounterWithTransactionAmountDifference'
)
const spyCounter = jest.spyOn(telemetryService, 'incrementCounter')

const createdPayment = await setup({
receiver,
debitAmount,
Expand All @@ -1179,6 +1233,8 @@ describe('OutgoingPaymentService', (): void => {
incomingPaymentReceived: payment.receiveAmount.value,
withdrawAmount: 0n
})
expect(spyTelFeeAmount).toHaveBeenCalledTimes(1)
expect(spyCounter).toHaveBeenCalledTimes(1)
})

test('COMPLETED (receiveAmount < incomingPayment.incomingAmount)', async (): Promise<void> => {
Expand Down
4 changes: 1 addition & 3 deletions packages/backend/src/open_payments/payment/outgoing/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,7 @@ async function createOutgoingPayment(
const peer = await deps.peerService.getByDestinationAddress(
receiver.ilpAddress
)
if (peer) {
await payment.$query(trx).patch({ peerId: peer.id })
}
if (peer) await payment.$query(trx).patch({ peerId: peer.id })

await sendWebhookEvent(
deps,
Expand Down
174 changes: 174 additions & 0 deletions packages/backend/src/telemetry/service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { mockCounter, mockHistogram } from '../tests/telemetry'
import { TelemetryService } from './service'
import { Counter, Histogram } from '@opentelemetry/api'
import { privacy } from './privacy'
import { mockRatesApi } from '../tests/rates'

jest.mock('@opentelemetry/api', () => ({
...jest.requireActual('@opentelemetry/api'),
Expand Down Expand Up @@ -36,6 +37,18 @@ describe('TelemetryServiceImpl', () => {
let aseRatesService: RatesService
let internalRatesService: RatesService

let apiRequestCount = 0
const exchangeRatesUrl = 'http://example-rates.com'

const exampleRates = {
USD: {
EUR: 2
},
EUR: {
USD: 1.12
}
}

beforeAll(async (): Promise<void> => {
deps = initIocContainer({
...Config,
Expand All @@ -49,6 +62,11 @@ describe('TelemetryServiceImpl', () => {
telemetryService = await deps.use('telemetry')!
aseRatesService = await deps.use('ratesService')!
internalRatesService = await deps.use('internalRatesService')!

mockRatesApi(exchangeRatesUrl, (base) => {
apiRequestCount++
return exampleRates[base as keyof typeof exampleRates]
})
})

afterAll(async (): Promise<void> => {
Expand Down Expand Up @@ -122,6 +140,162 @@ describe('TelemetryServiceImpl', () => {
expect(histogram?.record).toHaveBeenCalledTimes(2)
})

describe('incrementCounterWithTransactionAmountDifference', () => {
it('should not record fee when there is no fee value', async () => {
const spyAseConvert = jest.spyOn(aseRatesService, 'convert')
const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter')

await telemetryService.incrementCounterWithTransactionAmountDifference(
'test_amount_diff_counter',
{
value: 100n,
assetCode: 'USD',
assetScale: 2
},
{
value: 100n,
assetCode: 'USD',
assetScale: 2
}
)

expect(spyAseConvert).toHaveBeenCalled()
expect(spyIncCounter).not.toHaveBeenCalled()
})

it('should not record fee negative fee value', async () => {
const spyConvert = jest.spyOn(aseRatesService, 'convert')
const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter')

await telemetryService.incrementCounterWithTransactionAmountDifference(
'test_amount_diff_counter',
{
value: 100n,
assetCode: 'USD',
assetScale: 2
},
{
value: 101n,
assetCode: 'USD',
assetScale: 2
}
)

expect(spyConvert).toHaveBeenCalled()
expect(spyIncCounter).not.toHaveBeenCalled()
})

it('should not record zero amounts', async () => {
const spyConvert = jest.spyOn(aseRatesService, 'convert')
const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter')

await telemetryService.incrementCounterWithTransactionAmountDifference(
'test_amount_diff_counter',
{
value: 0n,
assetCode: 'USD',
assetScale: 2
},
{
value: 0n,
assetCode: 'USD',
assetScale: 2
}
)

expect(spyConvert).not.toHaveBeenCalled()
expect(spyIncCounter).not.toHaveBeenCalled()
})

it('should record since it is a valid fee', async () => {
const spyConvert = jest.spyOn(aseRatesService, 'convert')
const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter')

const source = {
value: 100n,
assetCode: 'USD',
assetScale: 2
}
const destination = {
value: 50n,
assetCode: 'USD',
assetScale: 2
}

const name = 'test_amount_diff_counter'
await telemetryService.incrementCounterWithTransactionAmountDifference(
name,
source,
destination
)

expect(spyConvert).toHaveBeenCalledTimes(2)
expect(spyConvert).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
sourceAmount: source.value,
sourceAsset: { code: source.assetCode, scale: source.assetScale }
})
)
expect(spyConvert).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
sourceAmount: destination.value,
sourceAsset: {
code: destination.assetCode,
scale: destination.assetScale
}
})
)
// Ensure the [incrementCounter] was called with the correct calculated value. Expected 5000 due to scale = 4.
expect(spyIncCounter).toHaveBeenCalledWith(name, 5000, {})
})

it('should record since it is a valid fee for different assets', async () => {
const spyConvert = jest.spyOn(aseRatesService, 'convert')
const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter')

const source = {
value: 100n,
assetCode: 'USD',
assetScale: 2
}
const destination = {
value: 50n,
assetCode: 'EUR',
assetScale: 2
}

const name = 'test_amount_diff_counter'
await telemetryService.incrementCounterWithTransactionAmountDifference(
name,
source,
destination
)

expect(spyConvert).toHaveBeenCalledTimes(2)
expect(spyConvert).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
sourceAmount: source.value,
sourceAsset: { code: source.assetCode, scale: source.assetScale }
})
)
expect(spyConvert).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
sourceAmount: destination.value,
sourceAsset: {
code: destination.assetCode,
scale: destination.assetScale
}
})
)
expect(spyIncCounter).toHaveBeenCalledWith(name, 4400, {})
expect(apiRequestCount).toBe(1)
})
})

describe('incrementCounterWithTransactionAmount', () => {
it('should try to convert using aseRatesService and fallback to internalRatesService', async () => {
const aseConvertSpy = jest
Expand Down
Loading

0 comments on commit 7a28fd1

Please sign in to comment.