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

refactor(backend): telemetry service interface #2807

Merged
merged 10 commits into from
Jul 22, 2024
96 changes: 96 additions & 0 deletions localenv/telemetry/grafana/provisioning/dashboards/example.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"editable": true,
"fiscalYearStartMonth": 0,
"graphTooltip": 0,
"id": 1,
"links": [],
"panels": [
{
Expand Down Expand Up @@ -215,6 +216,101 @@
],
"title": "Transaction Count",
"type": "timeseries"
},
{
"datasource": {
"type": "prometheus",
"uid": "PBFA97CFB590B2093"
},
"description": "Maximum time it takes to complete the fastest 25%/50%/75% of ILP payments.",
"fieldConfig": {
"defaults": {
"color": {
"mode": "thresholds"
},
"mappings": [],
"thresholds": {
"mode": "absolute",
"steps": [
{
"color": "green",
"value": null
},
{
"color": "red",
"value": 80
}
]
},
"unit": "ms"
},
"overrides": []
},
"gridPos": {
"h": 5,
"w": 12,
"x": 0,
"y": 16
},
"id": 3,
"options": {
"colorMode": "value",
"graphMode": "none",
"justifyMode": "auto",
"orientation": "auto",
"percentChangeColorMode": "standard",
"reduceOptions": {
"calcs": ["lastNotNull"],
"fields": "",
"values": false
},
"showPercentChange": false,
"textMode": "auto",
"wideLayout": true
},
"pluginVersion": "11.1.0",
"targets": [
{
"datasource": {
"type": "prometheus",
"uid": "PBFA97CFB590B2093"
},
"editorMode": "code",
"expr": "histogram_quantile(0.25, ilp_pay_time_ms_bucket)",
"instant": false,
"legendFormat": "First Quartile",
"range": true,
"refId": "A"
},
{
"datasource": {
"type": "prometheus",
"uid": "PBFA97CFB590B2093"
},
"editorMode": "code",
"expr": "histogram_quantile(0.50, ilp_pay_time_ms_bucket)",
"hide": false,
"instant": false,
"legendFormat": "Second Quartile",
"range": true,
"refId": "B"
},
{
"datasource": {
"type": "prometheus",
"uid": "PBFA97CFB590B2093"
},
"editorMode": "code",
"expr": "histogram_quantile(0.75, ilp_pay_time_ms_bucket)",
"hide": false,
"instant": false,
"legendFormat": "Third Quartile",
"range": true,
"refId": "C"
}
],
"title": "ILP Pay Time Quartile Approximations",
"type": "stat"
}
],
"refresh": "15s",
Expand Down
21 changes: 7 additions & 14 deletions packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,21 +85,14 @@ export async function handleSending(
const payEndTime = Date.now()

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

const payDuration = payEndTime - payStartTime
deps.telemetry
.getOrCreateHistogramMetric('ilp_pay_time_ms', {
description: 'Time to complete an ILP payment'
})
.record(payDuration, {
source: deps.telemetry.getInstanceName()
})
deps.telemetry.recordHistogram('ilp_pay_time_ms', payDuration, {
description: 'Time to complete an ILP payment'
})
}
Comment on lines +88 to 96
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should actually move these promises into a Promise.all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they arent promises 😄. the underlying add/record sdk methods are synchronous.

incrementCounterWithTransactionAmount returns a promise due to the amount conversion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's right. LGTM now


await handleCompleted(deps, payment)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { collectTelemetryAmount } from '../../../../../telemetry/transaction-amount'
import { ValueType } from '@opentelemetry/api'
import { ILPContext, ILPMiddleware } from '../rafiki'

export function createTelemetryMiddleware(): ILPMiddleware {
Expand All @@ -7,16 +7,24 @@ export function createTelemetryMiddleware(): ILPMiddleware {
next: () => Promise<void>
): Promise<void> => {
await next()
if (
services.telemetry &&
Number(request.prepare.amount) &&
response.fulfill
) {
const { code, scale } = accounts.outgoing.asset
collectTelemetryAmount(services.telemetry, services.logger, {
amount: BigInt(request.prepare.amount),
asset: { code: code, scale: scale }
})

const value = BigInt(request.prepare.amount)

if (services.telemetry && value && response.fulfill) {
const { code: assetCode, scale: assetScale } = accounts.outgoing.asset

await services.telemetry.incrementCounterWithTransactionAmount(
'transactions_amount',
{
value,
assetCode,
assetScale
},
{
description: 'Amount sent through the network',
valueType: ValueType.DOUBLE
}
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ import { IlpResponse, OutgoingAccount, ZeroCopyIlpPrepare } from '../..'
import { IncomingAccountFactory, RafikiServicesFactory } from '../../factories'
import { createTelemetryMiddleware } from '../../middleware/telemetry'
import { createILPContext } from '../../utils'

import { IlpFulfill } from 'ilp-packet'
import * as telemetry from '../../../../../../telemetry/transaction-amount'

const incomingAccount = IncomingAccountFactory.build({ id: 'alice' })

Expand Down Expand Up @@ -35,7 +33,6 @@ const ctx = createILPContext({
} as IlpResponse
})

jest.mock('../../../../../../telemetry/transaction-amount')
const middleware = createTelemetryMiddleware()
const next = jest.fn().mockImplementation(() => Promise.resolve())

Expand All @@ -46,33 +43,38 @@ beforeEach(async () => {
})

describe('Telemetry Middleware', function () {
it('does not gather telemetry if telemetry is not enabled (service is undefined)', async () => {
const collectAmountSpy = jest
.spyOn(telemetry, 'collectTelemetryAmount')
it('should not gather telemetry if telemetry is not enabled (service is undefined)', async () => {
const incrementCounterWithTransactionAmountSpy = jest
.spyOn(services.telemetry, 'incrementCounterWithTransactionAmount')
.mockImplementation(() => Promise.resolve())

await middleware(
{ ...ctx, services: { ...ctx.services, telemetry: undefined } },
next
)
expect(collectAmountSpy).not.toHaveBeenCalled()

expect(incrementCounterWithTransactionAmountSpy).not.toHaveBeenCalled()
expect(next).toHaveBeenCalled()
})

it('does not gather telemetry if response.fulfill undefined', async () => {
const collectAmountSpy = jest.spyOn(telemetry, 'collectTelemetryAmount')
it('should not gather telemetry if response.fulfill undefined', async () => {
const incrementCounterWithTransactionAmountSpy = jest
.spyOn(services.telemetry, 'incrementCounterWithTransactionAmount')
.mockImplementation(() => Promise.resolve())

await middleware(
{ ...ctx, response: { fulfill: undefined } as IlpResponse },
next
)

expect(collectAmountSpy).not.toHaveBeenCalled()
expect(incrementCounterWithTransactionAmountSpy).not.toHaveBeenCalled()
expect(next).toHaveBeenCalled()
})

it('does not gather telemetry if amount is invalid', async () => {
const collectAmountSpy = jest.spyOn(telemetry, 'collectTelemetryAmount')
it('should not gather telemetry if request.prepare.amount is 0', async () => {
const incrementCounterWithTransactionAmountSpy = jest
.spyOn(services.telemetry, 'incrementCounterWithTransactionAmount')
.mockImplementation(() => Promise.resolve())

await middleware(
{
Expand All @@ -85,27 +87,27 @@ describe('Telemetry Middleware', function () {
next
)

expect(collectAmountSpy).not.toHaveBeenCalled()
expect(incrementCounterWithTransactionAmountSpy).not.toHaveBeenCalled()
expect(next).toHaveBeenCalled()
})

it('gathers telemetry without blocking middleware chain', async () => {
it('should gather telemetry without blocking middleware chain', async () => {
let nextCalled = false
const next = jest.fn().mockImplementation(() => {
nextCalled = true
return Promise.resolve()
})

const collectAmountSpy = jest
.spyOn(telemetry, 'collectTelemetryAmount')
const incrementCounterWithTransactionAmountSpy = jest
.spyOn(services.telemetry, 'incrementCounterWithTransactionAmount')
.mockImplementation(() => {
expect(nextCalled).toBe(true)
return Promise.resolve()
})

await middleware(ctx, next)

expect(collectAmountSpy).toHaveBeenCalled()
expect(incrementCounterWithTransactionAmountSpy).toHaveBeenCalled()
expect(next).toHaveBeenCalled()
})
})
Loading
Loading