From 5de7ad92e20ca791d4afd1b545c9ffd82754bfaf Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:35:27 -0400 Subject: [PATCH 1/4] refactor(backend): add NoopTelemetryServiceImpl to make tel optional --- packages/backend/src/app.ts | 4 +- packages/backend/src/index.ts | 76 +- .../backend/src/telemetry/service.test.ts | 705 +++++++++--------- packages/backend/src/telemetry/service.ts | 62 +- 4 files changed, 462 insertions(+), 385 deletions(-) diff --git a/packages/backend/src/app.ts b/packages/backend/src/app.ts index c56544f38f..d0cdd19f35 100644 --- a/packages/backend/src/app.ts +++ b/packages/backend/src/app.ts @@ -215,8 +215,8 @@ const WALLET_ADDRESS_PATH = '/:walletAddressPath+' export interface AppServices { logger: Promise - telemetry?: Promise - internalRatesService?: Promise + telemetry: Promise + internalRatesService: Promise knex: Promise axios: Promise config: Promise diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index a34d784ef7..fd60a245e5 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -47,14 +47,11 @@ import { } from './payment-method/ilp/ilp_plugin' import { createHttpTokenService } from './payment-method/ilp/peer-http-token/service' import { createPeerService } from './payment-method/ilp/peer/service' -import { - createIlpPaymentService, - ServiceDependencies as IlpPaymentServiceDependencies -} from './payment-method/ilp/service' +import { createIlpPaymentService } from './payment-method/ilp/service' import { createSPSPRoutes } from './payment-method/ilp/spsp/routes' import { createStreamCredentialsService } from './payment-method/ilp/stream-credentials/service' import { createRatesService } from './rates/service' -import { TelemetryService, createTelemetryService } from './telemetry/service' +import { createTelemetryService } from './telemetry/service' import { createWebhookService } from './webhook/service' BigInt.prototype.toJSON = function () { @@ -135,29 +132,28 @@ export function initIocContainer( }) }) - if (config.enableTelemetry) { - container.singleton('internalRatesService', async (deps) => { - return createRatesService({ - logger: await deps.use('logger'), - exchangeRatesUrl: config.telemetryExchangeRatesUrl, - exchangeRatesLifetime: config.telemetryExchangeRatesLifetime - }) + container.singleton('internalRatesService', async (deps) => { + return createRatesService({ + logger: await deps.use('logger'), + exchangeRatesUrl: config.telemetryExchangeRatesUrl, + exchangeRatesLifetime: config.telemetryExchangeRatesLifetime }) + }) - container.singleton('telemetry', async (deps) => { - const config = await deps.use('config') - return createTelemetryService({ - logger: await deps.use('logger'), - aseRatesService: await deps.use('ratesService'), - internalRatesService: await deps.use('internalRatesService')!, - instanceName: config.instanceName, - collectorUrls: config.openTelemetryCollectors, - exportIntervalMillis: config.openTelemetryExportInterval, - baseAssetCode: 'USD', - baseScale: 4 - }) + container.singleton('telemetry', async (deps) => { + const config = await deps.use('config') + return createTelemetryService({ + logger: await deps.use('logger'), + aseRatesService: await deps.use('ratesService'), + internalRatesService: await deps.use('internalRatesService')!, + instanceName: config.instanceName, + collectorUrls: config.openTelemetryCollectors, + exportIntervalMillis: config.openTelemetryExportInterval, + enableTelemetry: config.enableTelemetry, + baseAssetCode: 'USD', + baseScale: 4 }) - } + }) container.singleton('openApi', async () => { const resourceServerSpec = await getResourceServerOpenAPI() @@ -356,10 +352,6 @@ export function initIocContainer( container.singleton('connectorApp', async (deps) => { const config = await deps.use('config') - let telemetry: TelemetryService | undefined - if (config.enableTelemetry) { - telemetry = await deps.use('telemetry') - } return await createConnectorService({ logger: await deps.use('logger'), redis: await deps.use('redis'), @@ -370,7 +362,7 @@ export function initIocContainer( ratesService: await deps.use('ratesService'), streamServer: await deps.use('streamServer'), ilpAddress: config.ilpAddress, - telemetry + telemetry: await deps.use('telemetry') }) }) @@ -425,19 +417,14 @@ export function initIocContainer( }) container.singleton('ilpPaymentService', async (deps) => { - const serviceDependencies: IlpPaymentServiceDependencies = { + return await createIlpPaymentService({ logger: await deps.use('logger'), knex: await deps.use('knex'), config: await deps.use('config'), makeIlpPlugin: await deps.use('makeIlpPlugin'), - ratesService: await deps.use('ratesService') - } - - if (config.enableTelemetry) { - serviceDependencies.telemetry = await deps.use('telemetry') - } - - return createIlpPaymentService(serviceDependencies) + ratesService: await deps.use('ratesService'), + telemetry: await deps.use('telemetry') + }) }) container.singleton('paymentMethodHandlerService', async (deps) => { @@ -469,7 +456,6 @@ export function initIocContainer( }) container.singleton('outgoingPaymentService', async (deps) => { - const config = await deps.use('config') return await createOutgoingPaymentService({ logger: await deps.use('logger'), knex: await deps.use('knex'), @@ -481,9 +467,7 @@ export function initIocContainer( peerService: await deps.use('peerService'), walletAddressService: await deps.use('walletAddressService'), quoteService: await deps.use('quoteService'), - telemetry: config.enableTelemetry - ? await deps.use('telemetry') - : undefined + telemetry: await deps.use('telemetry') }) }) @@ -548,10 +532,8 @@ export const gracefulShutdown = async ( await redis.quit() redis.disconnect() - if (config.enableTelemetry) { - const telemetry = await container.use('telemetry') - telemetry?.shutdown() - } + const telemetry = await container.use('telemetry') + telemetry.shutdown() } export const start = async ( diff --git a/packages/backend/src/telemetry/service.test.ts b/packages/backend/src/telemetry/service.test.ts index 3834212f01..e507ff470c 100644 --- a/packages/backend/src/telemetry/service.test.ts +++ b/packages/backend/src/telemetry/service.test.ts @@ -5,7 +5,13 @@ import { Config } from '../config/app' import { ConvertError, RatesService } from '../rates/service' import { TestContainer, createTestApp } from '../tests/app' import { mockCounter, mockHistogram } from '../tests/telemetry' -import { TelemetryService } from './service' +import { + createTelemetryService, + NoopTelemetryServiceImpl, + TelemetryService, + TelemetryServiceImpl, + TelemetryServiceDependencies +} from './service' import { Counter, Histogram } from '@opentelemetry/api' import { privacy } from './privacy' import { mockRatesApi } from '../tests/rates' @@ -30,406 +36,439 @@ jest.mock('@opentelemetry/sdk-metrics', () => ({ })) })) -describe('TelemetryServiceImpl', () => { - let deps: IocContract - let appContainer: TestContainer - let telemetryService: TelemetryService - let aseRatesService: RatesService - let internalRatesService: RatesService - - let apiRequestCount = 0 - const exchangeRatesUrl = 'http://example-rates.com' - - const exampleRates = { - USD: { - EUR: 2 - }, - EUR: { - USD: 1.12 +describe('Telemetry Service', () => { + describe('Telemtry Enabled', () => { + let deps: IocContract + let appContainer: TestContainer + let telemetryService: TelemetryService + 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 => { - deps = initIocContainer({ - ...Config, - enableTelemetry: true, - telemetryExchangeRatesUrl: 'http://example-rates.com', - telemetryExchangeRatesLifetime: 100, - openTelemetryCollectors: [] - }) + beforeAll(async (): Promise => { + deps = initIocContainer({ + ...Config, + enableTelemetry: true, + telemetryExchangeRatesUrl: 'http://example-rates.com', + telemetryExchangeRatesLifetime: 100, + openTelemetryCollectors: [] + }) - appContainer = await createTestApp(deps) - telemetryService = await deps.use('telemetry')! - aseRatesService = await deps.use('ratesService')! - internalRatesService = await deps.use('internalRatesService')! + appContainer = await createTestApp(deps) + 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] + mockRatesApi(exchangeRatesUrl, (base) => { + apiRequestCount++ + return exampleRates[base as keyof typeof exampleRates] + }) }) - }) - afterAll(async (): Promise => { - await appContainer.shutdown() - }) + afterAll(async (): Promise => { + await appContainer.shutdown() + }) - it('should create a counter with source attribute for a new metric', () => { - const name = 'test_counter' - const amount = 1 - const attributes = { test: 'attribute' } + it('should create a counter with source attribute for a new metric', () => { + const name = 'test_counter' + const amount = 1 + const attributes = { test: 'attribute' } - telemetryService.incrementCounter(name, amount, attributes) + telemetryService.incrementCounter(name, amount, attributes) - expect(mockCounter.add).toHaveBeenCalledWith( - amount, - expect.objectContaining({ - ...attributes, - source: expect.any(String) - }) - ) - }) + expect(mockCounter.add).toHaveBeenCalledWith( + amount, + expect.objectContaining({ + ...attributes, + source: expect.any(String) + }) + ) + }) - it('should create a histogram with source attribute for a new metric', () => { - const name = 'test_histogram' - const amount = 1 - const attributes = { test: 'attribute' } + it('should create a histogram with source attribute for a new metric', () => { + const name = 'test_histogram' + const amount = 1 + const attributes = { test: 'attribute' } - telemetryService.recordHistogram(name, amount, attributes) + telemetryService.recordHistogram(name, amount, attributes) - expect(mockHistogram.record).toHaveBeenCalledWith( - amount, - expect.objectContaining({ - ...attributes, - source: expect.any(String) - }) - ) - }) + expect(mockHistogram.record).toHaveBeenCalledWith( + amount, + expect.objectContaining({ + ...attributes, + source: expect.any(String) + }) + ) + }) - it('should use existing counter when incrementCounter is called for an existing metric', () => { - const name = 'test_counter' + it('should use existing counter when incrementCounter is called for an existing metric', () => { + const name = 'test_counter' - telemetryService.incrementCounter(name, 1) - telemetryService.incrementCounter(name, 1) + telemetryService.incrementCounter(name, 1) + telemetryService.incrementCounter(name, 1) - //"any" to access private ts class member variable - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const counters: Map = (telemetryService as any).counters + //"any" to access private ts class member variable + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const counters: Map = (telemetryService as any).counters - expect(counters.size).toBe(1) - expect(counters.has(name)).toBe(true) + expect(counters.size).toBe(1) + expect(counters.has(name)).toBe(true) - const counter = counters.get(name) - expect(counter?.add).toHaveBeenCalledTimes(2) - }) + const counter = counters.get(name) + expect(counter?.add).toHaveBeenCalledTimes(2) + }) - it('should use existing histogram when recordHistogram is called for an existing metric', () => { - const name = 'test_histogram' + it('should use existing histogram when recordHistogram is called for an existing metric', () => { + const name = 'test_histogram' - telemetryService.recordHistogram(name, 1) - telemetryService.recordHistogram(name, 1) + telemetryService.recordHistogram(name, 1) + telemetryService.recordHistogram(name, 1) - //"any" to access private ts class member variable - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const histograms: Map = (telemetryService as any) - .histograms + //"any" to access private ts class member variable + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const histograms: Map = (telemetryService as any) + .histograms - expect(histograms.size).toBe(1) - expect(histograms.has(name)).toBe(true) + expect(histograms.size).toBe(1) + expect(histograms.has(name)).toBe(true) - const histogram = histograms.get(name) - expect(histogram?.record).toHaveBeenCalledTimes(2) - }) + const histogram = histograms.get(name) + 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') + 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 + } + ) - 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() + }) - 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 not record fee negative fee value', async () => { - const spyConvert = jest.spyOn(aseRatesService, 'convert') - const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter') + it('should record since it is a valid fee', async () => { + const spyConvert = jest.spyOn(aseRatesService, 'convert') + const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter') - await telemetryService.incrementCounterWithTransactionAmountDifference( - 'test_amount_diff_counter', - { + const source = { value: 100n, assetCode: 'USD', assetScale: 2 - }, - { - value: 101n, + } + const destination = { + value: 50n, assetCode: 'USD', assetScale: 2 } - ) - expect(spyConvert).toHaveBeenCalled() - expect(spyIncCounter).not.toHaveBeenCalled() - }) + 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 not record zero amounts', async () => { - const spyConvert = jest.spyOn(aseRatesService, 'convert') - const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter') + 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') - await telemetryService.incrementCounterWithTransactionAmountDifference( - 'test_amount_diff_counter', - { - value: 0n, + const source = { + value: 100n, assetCode: 'USD', assetScale: 2 - }, - { - value: 0n, - assetCode: 'USD', + } + const destination = { + value: 50n, + assetCode: 'EUR', assetScale: 2 } - ) - expect(spyConvert).not.toHaveBeenCalled() - expect(spyIncCounter).not.toHaveBeenCalled() - }) + const name = 'test_amount_diff_counter' + await telemetryService.incrementCounterWithTransactionAmountDifference( + name, + source, + destination + ) - it('should record since it is a valid fee', async () => { - const spyConvert = jest.spyOn(aseRatesService, 'convert') - const spyIncCounter = jest.spyOn(telemetryService, 'incrementCounter') + 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) + }) + }) - const source = { - value: 100n, - assetCode: 'USD', - assetScale: 2 - } - const destination = { - value: 50n, - assetCode: 'USD', - assetScale: 2 - } + describe('incrementCounterWithTransactionAmount', () => { + it('should try to convert using aseRatesService and fallback to internalRatesService', async () => { + const aseConvertSpy = jest + .spyOn(aseRatesService, 'convert') + .mockImplementation(() => + Promise.resolve(ConvertError.InvalidDestinationPrice) + ) + const internalConvertSpy = jest + .spyOn(internalRatesService, 'convert') + .mockImplementation(() => Promise.resolve(10000n)) + + await telemetryService.incrementCounterWithTransactionAmount( + 'test_counter', + { + value: 100n, + assetCode: 'USD', + assetScale: 2 + } + ) - const name = 'test_amount_diff_counter' - await telemetryService.incrementCounterWithTransactionAmountDifference( - name, - source, - destination - ) + expect(aseConvertSpy).toHaveBeenCalled() + expect(internalConvertSpy).toHaveBeenCalled() + }) - 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 + it('should not call the fallback internalRatesService if aseRatesService call is successful', async () => { + const aseConvertSpy = jest + .spyOn(aseRatesService, 'convert') + .mockImplementation(() => Promise.resolve(500n)) + const internalConvertSpy = jest.spyOn(internalRatesService, 'convert') + + await telemetryService.incrementCounterWithTransactionAmount( + 'test_counter', + { + value: 100n, + assetCode: 'USD', + assetScale: 2 } - }) - ) - // 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 - } + expect(aseConvertSpy).toHaveBeenCalled() + expect(internalConvertSpy).not.toHaveBeenCalled() + }) - const name = 'test_amount_diff_counter' - await telemetryService.incrementCounterWithTransactionAmountDifference( - name, - source, - destination - ) + it('should apply privacy', async () => { + const convertedAmount = 500n + + jest + //"any" to access private ts class member variable + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .spyOn(telemetryService as any, 'convertAmount') + .mockImplementation(() => Promise.resolve(convertedAmount)) + const applyPrivacySpy = jest + .spyOn(privacy, 'applyPrivacy') + .mockReturnValue(123) + const incrementCounterSpy = jest.spyOn( + telemetryService, + 'incrementCounter' + ) - 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 + const counterName = 'test_counter' + await telemetryService.incrementCounterWithTransactionAmount( + counterName, + { + value: 100n, + assetCode: 'USD', + assetScale: 2 } - }) - ) - 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 - .spyOn(aseRatesService, 'convert') - .mockImplementation(() => - Promise.resolve(ConvertError.InvalidDestinationPrice) ) - const internalConvertSpy = jest - .spyOn(internalRatesService, 'convert') - .mockImplementation(() => Promise.resolve(10000n)) - await telemetryService.incrementCounterWithTransactionAmount( - 'test_counter', - { - value: 100n, - assetCode: 'USD', - assetScale: 2 - } - ) - - expect(aseConvertSpy).toHaveBeenCalled() - expect(internalConvertSpy).toHaveBeenCalled() - }) + expect(applyPrivacySpy).toHaveBeenCalledWith(Number(convertedAmount)) + expect(incrementCounterSpy).toHaveBeenCalledWith( + counterName, + 123, + expect.any(Object) + ) + }) - it('should not call the fallback internalRatesService if aseRatesService call is successful', async () => { - const aseConvertSpy = jest - .spyOn(aseRatesService, 'convert') - .mockImplementation(() => Promise.resolve(500n)) - const internalConvertSpy = jest.spyOn(internalRatesService, 'convert') + it('should not collect telemetry when conversion returns InvalidDestinationPrice', async () => { + const convertSpy = jest + //"any" to access private ts class member variable + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .spyOn(telemetryService as any, 'convertAmount') + .mockImplementation(() => + Promise.resolve(ConvertError.InvalidDestinationPrice) + ) + + const incrementCounterSpy = jest.spyOn( + telemetryService, + 'incrementCounter' + ) - await telemetryService.incrementCounterWithTransactionAmount( - 'test_counter', - { - value: 100n, - assetCode: 'USD', - assetScale: 2 - } - ) + await telemetryService.incrementCounterWithTransactionAmount( + 'test_counter', + { + value: 100n, + assetCode: 'USD', + assetScale: 2 + } + ) - expect(aseConvertSpy).toHaveBeenCalled() - expect(internalConvertSpy).not.toHaveBeenCalled() - }) + expect(convertSpy).toHaveBeenCalled() + expect(incrementCounterSpy).not.toHaveBeenCalled() + }) - it('should apply privacy', async () => { - const convertedAmount = 500n - - jest - //"any" to access private ts class member variable - // eslint-disable-next-line @typescript-eslint/no-explicit-any - .spyOn(telemetryService as any, 'convertAmount') - .mockImplementation(() => Promise.resolve(convertedAmount)) - const applyPrivacySpy = jest - .spyOn(privacy, 'applyPrivacy') - .mockReturnValue(123) - const incrementCounterSpy = jest.spyOn( - telemetryService, - 'incrementCounter' - ) + it('should collect telemetry when conversion is successful', async () => { + const convertSpy = jest + //"any" to access private ts class member variable + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .spyOn(telemetryService as any, 'convertAmount') + .mockImplementation(() => Promise.resolve(10000n)) + const incrementCounterSpy = jest.spyOn( + telemetryService, + 'incrementCounter' + ) + const obfuscatedAmount = 12000 + jest.spyOn(privacy, 'applyPrivacy').mockReturnValue(obfuscatedAmount) - const counterName = 'test_counter' - await telemetryService.incrementCounterWithTransactionAmount( - counterName, - { - value: 100n, - assetCode: 'USD', - assetScale: 2 - } - ) + const counterName = 'test_counter' - expect(applyPrivacySpy).toHaveBeenCalledWith(Number(convertedAmount)) - expect(incrementCounterSpy).toHaveBeenCalledWith( - counterName, - 123, - expect.any(Object) - ) - }) + await telemetryService.incrementCounterWithTransactionAmount( + counterName, + { + value: 100n, + assetCode: 'USD', + assetScale: 2 + } + ) - it('should not collect telemetry when conversion returns InvalidDestinationPrice', async () => { - const convertSpy = jest - //"any" to access private ts class member variable - // eslint-disable-next-line @typescript-eslint/no-explicit-any - .spyOn(telemetryService as any, 'convertAmount') - .mockImplementation(() => - Promise.resolve(ConvertError.InvalidDestinationPrice) + expect(convertSpy).toHaveBeenCalled() + expect(incrementCounterSpy).toHaveBeenCalledWith( + counterName, + obfuscatedAmount, + expect.any(Object) ) + }) + }) + }) + describe('Telemetry Disabled', () => { + let deps: TelemetryServiceDependencies - const incrementCounterSpy = jest.spyOn( - telemetryService, - 'incrementCounter' - ) + beforeEach(() => { + deps = { + enableTelemetry: false + } as TelemetryServiceDependencies + }) - await telemetryService.incrementCounterWithTransactionAmount( - 'test_counter', - { - value: 100n, - assetCode: 'USD', - assetScale: 2 - } - ) + test('should return NoopTelemetryServiceImpl when enableTelemetry is false', () => { + const telemetryService = createTelemetryService(deps) - expect(convertSpy).toHaveBeenCalled() - expect(incrementCounterSpy).not.toHaveBeenCalled() + expect(telemetryService).toBeInstanceOf(NoopTelemetryServiceImpl) }) - it('should collect telemetry when conversion is successful', async () => { - const convertSpy = jest - //"any" to access private ts class member variable - // eslint-disable-next-line @typescript-eslint/no-explicit-any - .spyOn(telemetryService as any, 'convertAmount') - .mockImplementation(() => Promise.resolve(10000n)) - const incrementCounterSpy = jest.spyOn( - telemetryService, - 'incrementCounter' - ) - const obfuscatedAmount = 12000 - jest.spyOn(privacy, 'applyPrivacy').mockReturnValue(obfuscatedAmount) + test('should return TelemetryServiceImpl when enableTelemetry is true', () => { + deps.enableTelemetry = true + const telemetryService = createTelemetryService(deps) - const counterName = 'test_counter' + expect(telemetryService).toBeInstanceOf(TelemetryServiceImpl) + }) - await telemetryService.incrementCounterWithTransactionAmount( - counterName, - { - value: 100n, - assetCode: 'USD', - assetScale: 2 - } - ) + test('NoopTelemetryServiceImpl should not get meter ', () => { + const telemetryService = createTelemetryService(deps) + telemetryService.recordHistogram('testhistogram', 1) + telemetryService.incrementCounter('testcounter', 1) - expect(convertSpy).toHaveBeenCalled() - expect(incrementCounterSpy).toHaveBeenCalledWith( - counterName, - obfuscatedAmount, - expect.any(Object) - ) + expect(mockCounter.add).toHaveBeenCalledTimes(0) + expect(mockHistogram.record).toHaveBeenCalledTimes(0) }) }) }) diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index d0c03435a9..13d4eb6481 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -32,7 +32,7 @@ export interface TelemetryService { ): void } -interface TelemetryServiceDependencies extends BaseService { +export interface TelemetryServiceDependencies extends BaseService { instanceName: string collectorUrls: string[] exportIntervalMillis?: number @@ -40,6 +40,7 @@ interface TelemetryServiceDependencies extends BaseService { internalRatesService: RatesService baseAssetCode: string baseScale: number + enableTelemetry: boolean } const METER_NAME = 'Rafiki' @@ -47,10 +48,13 @@ const METER_NAME = 'Rafiki' export function createTelemetryService( deps: TelemetryServiceDependencies ): TelemetryService { + if (!deps.enableTelemetry) { + return new NoopTelemetryServiceImpl(deps) + } return new TelemetryServiceImpl(deps) } -class TelemetryServiceImpl implements TelemetryService { +export class TelemetryServiceImpl implements TelemetryService { private instanceName: string private meterProvider?: MeterProvider private internalRatesService: RatesService @@ -178,7 +182,7 @@ class TelemetryServiceImpl implements TelemetryService { public recordHistogram( name: string, value: number, - attributes: Record = {} + attributes?: Record ): void { const histogram = this.getOrCreateHistogram(name) histogram.record(value, { @@ -216,3 +220,55 @@ class TelemetryServiceImpl implements TelemetryService { return converted } } + +/* eslint-disable @typescript-eslint/no-unused-vars */ +export class NoopTelemetryServiceImpl implements TelemetryService { + private instanceName: string + private meterProvider?: MeterProvider + private internalRatesService: RatesService + private aseRatesService: RatesService + + constructor(private deps: TelemetryServiceDependencies) { + this.instanceName = deps.instanceName + this.internalRatesService = deps.internalRatesService + this.aseRatesService = deps.aseRatesService + } + + public async shutdown(): Promise { + // do nothing + } + + public incrementCounter( + name: string, + value: number, + attributes?: Record + ): void { + // do nothing + } + + public recordHistogram( + name: string, + value: number, + attributes?: Record + ): void { + // do nothing + } + + public async incrementCounterWithTransactionAmountDifference( + name: string, + amountSource: { value: bigint; assetCode: string; assetScale: number }, + amountDestination: { value: bigint; assetCode: string; assetScale: number }, + attributes?: Record + ): Promise { + // do nothing + } + + public async incrementCounterWithTransactionAmount( + name: string, + amount: { value: bigint; assetCode: string; assetScale: number }, + attributes: Record = {}, + preservePrivacy = true + ): Promise { + // do nothing + } +} From 751ffa96d1f1ea15eb298bdc0cdb15496b37071e Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:59:56 -0400 Subject: [PATCH 2/4] refactor(backend): rm unecessary telemetry exists check --- .../payment/outgoing/lifecycle.ts | 38 +++++++++--------- .../open_payments/payment/outgoing/service.ts | 2 +- .../ilp/connector/core/rafiki.ts | 40 +++++++++---------- .../src/payment-method/ilp/connector/index.ts | 2 +- .../backend/src/payment-method/ilp/service.ts | 20 +++++----- 5 files changed, 47 insertions(+), 55 deletions(-) diff --git a/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts b/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts index f273e008a1..1b402db2d3 100644 --- a/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts +++ b/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts @@ -85,26 +85,24 @@ export async function handleSending( }) const payEndTime = Date.now() - if (deps.telemetry) { - const payDuration = payEndTime - payStartTime - 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 - } - ) - ]) - } + const payDuration = payEndTime - payStartTime + 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) } diff --git a/packages/backend/src/open_payments/payment/outgoing/service.ts b/packages/backend/src/open_payments/payment/outgoing/service.ts index 050700aca1..336444e518 100644 --- a/packages/backend/src/open_payments/payment/outgoing/service.ts +++ b/packages/backend/src/open_payments/payment/outgoing/service.ts @@ -66,7 +66,7 @@ export interface ServiceDependencies extends BaseService { paymentMethodHandlerService: PaymentMethodHandlerService walletAddressService: WalletAddressService quoteService: QuoteService - telemetry?: TelemetryService + telemetry: TelemetryService } export async function createOutgoingPaymentService( diff --git a/packages/backend/src/payment-method/ilp/connector/core/rafiki.ts b/packages/backend/src/payment-method/ilp/connector/core/rafiki.ts index a6095fb064..6740d21886 100644 --- a/packages/backend/src/payment-method/ilp/connector/core/rafiki.ts +++ b/packages/backend/src/payment-method/ilp/connector/core/rafiki.ts @@ -63,7 +63,7 @@ export interface TransferOptions { export interface RafikiServices { //router: Router accounting: AccountingService - telemetry?: TelemetryService + telemetry: TelemetryService walletAddresses: WalletAddressService logger: Logger incomingPayments: IncomingPaymentService @@ -143,7 +143,7 @@ export class Rafiki { get walletAddresses(): WalletAddressService { return config.walletAddresses }, - get telemetry(): TelemetryService | undefined { + get telemetry(): TelemetryService { return config.telemetry }, logger @@ -162,9 +162,7 @@ export class Rafiki { const response = new IlpResponse() const telemetry = this.publicServer.context.services.telemetry - if (telemetry) { - incrementPreparePacketCount(unfulfillable, prepare.amount, telemetry) - } + incrementPreparePacketCount(unfulfillable, prepare.amount, telemetry) await this.routes( { @@ -194,23 +192,21 @@ export class Rafiki { ) if (!response.rawReply) throw new Error('error generating reply') - if (telemetry) { - const { code, scale } = sourceAccount.asset - incrementFulfillOrRejectPacketCount( - unfulfillable, - prepare.amount, - response, - telemetry - ) - await incrementAmount( - unfulfillable, - prepare.amount, - response, - code, - scale, - telemetry - ) - } + const { code, scale } = sourceAccount.asset + incrementFulfillOrRejectPacketCount( + unfulfillable, + prepare.amount, + response, + telemetry + ) + await incrementAmount( + unfulfillable, + prepare.amount, + response, + code, + scale, + telemetry + ) return response.rawReply } diff --git a/packages/backend/src/payment-method/ilp/connector/index.ts b/packages/backend/src/payment-method/ilp/connector/index.ts index 9f3ecc3f95..8dd2f27f0d 100644 --- a/packages/backend/src/payment-method/ilp/connector/index.ts +++ b/packages/backend/src/payment-method/ilp/connector/index.ts @@ -38,7 +38,7 @@ interface ServiceDependencies extends BaseService { peerService: PeerService streamServer: StreamServer ilpAddress: string - telemetry?: TelemetryService + telemetry: TelemetryService } export async function createConnectorService({ diff --git a/packages/backend/src/payment-method/ilp/service.ts b/packages/backend/src/payment-method/ilp/service.ts index 4974ffab24..b21c80db32 100644 --- a/packages/backend/src/payment-method/ilp/service.ts +++ b/packages/backend/src/payment-method/ilp/service.ts @@ -22,7 +22,7 @@ export interface ServiceDependencies extends BaseService { config: IAppConfig ratesService: RatesService makeIlpPlugin: (options: IlpPluginOptions) => IlpPlugin - telemetry?: TelemetryService + telemetry: TelemetryService } export async function createIlpPaymentService( @@ -94,16 +94,14 @@ async function getQuote( } const payEndTime = Date.now() - if (deps.telemetry) { - const rateProbeDuraiton = payEndTime - rateProbeStartTime - deps.telemetry.recordHistogram( - 'ilp_rate_probe_time_ms', - rateProbeDuraiton, - { - description: 'Time to get an ILP quote' - } - ) - } + const rateProbeDuraiton = payEndTime - rateProbeStartTime + deps.telemetry.recordHistogram( + 'ilp_rate_probe_time_ms', + rateProbeDuraiton, + { + description: 'Time to get an ILP quote' + } + ) // Pay.startQuote should return PaymentError.InvalidSourceAmount or // PaymentError.InvalidDestinationAmount for non-positive amounts. // Outgoing payments' sendAmount or receiveAmount should never be From a003c3ed9a9784e4e99212bd1b2e6098f7a70045 Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Thu, 19 Sep 2024 14:08:38 -0400 Subject: [PATCH 3/4] fix(localenv): rm telemetry log noise --- localenv/telemetry/otel-collector-config.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/localenv/telemetry/otel-collector-config.yaml b/localenv/telemetry/otel-collector-config.yaml index 32e08f8902..d577a0ec78 100644 --- a/localenv/telemetry/otel-collector-config.yaml +++ b/localenv/telemetry/otel-collector-config.yaml @@ -8,6 +8,8 @@ processors: batch: exporters: + logging: + loglevel: info debug: verbosity: detailed prometheus: @@ -18,6 +20,9 @@ exporters: insecure: true service: + telemetry: + logs: + level: warn pipelines: metrics: receivers: [otlp] From dfd8b2b04e54920433756aee169fa3d831baae9f Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Tue, 24 Sep 2024 14:09:37 -0400 Subject: [PATCH 4/4] feat(backend): switch tel servive impl in dep definition --- packages/backend/src/index.ts | 11 ++++- .../backend/src/telemetry/service.test.ts | 41 ++++++++++--------- packages/backend/src/telemetry/service.ts | 19 +++------ 3 files changed, 35 insertions(+), 36 deletions(-) diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index fd60a245e5..dfa64c0ef4 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -51,7 +51,10 @@ import { createIlpPaymentService } from './payment-method/ilp/service' import { createSPSPRoutes } from './payment-method/ilp/spsp/routes' import { createStreamCredentialsService } from './payment-method/ilp/stream-credentials/service' import { createRatesService } from './rates/service' -import { createTelemetryService } from './telemetry/service' +import { + createTelemetryService, + createNoopTelemetryService +} from './telemetry/service' import { createWebhookService } from './webhook/service' BigInt.prototype.toJSON = function () { @@ -142,6 +145,11 @@ export function initIocContainer( container.singleton('telemetry', async (deps) => { const config = await deps.use('config') + + if (!config.enableTelemetry) { + return createNoopTelemetryService() + } + return createTelemetryService({ logger: await deps.use('logger'), aseRatesService: await deps.use('ratesService'), @@ -149,7 +157,6 @@ export function initIocContainer( instanceName: config.instanceName, collectorUrls: config.openTelemetryCollectors, exportIntervalMillis: config.openTelemetryExportInterval, - enableTelemetry: config.enableTelemetry, baseAssetCode: 'USD', baseScale: 4 }) diff --git a/packages/backend/src/telemetry/service.test.ts b/packages/backend/src/telemetry/service.test.ts index e507ff470c..da0ae289cb 100644 --- a/packages/backend/src/telemetry/service.test.ts +++ b/packages/backend/src/telemetry/service.test.ts @@ -6,11 +6,9 @@ import { ConvertError, RatesService } from '../rates/service' import { TestContainer, createTestApp } from '../tests/app' import { mockCounter, mockHistogram } from '../tests/telemetry' import { - createTelemetryService, NoopTelemetryServiceImpl, TelemetryService, - TelemetryServiceImpl, - TelemetryServiceDependencies + TelemetryServiceImpl } from './service' import { Counter, Histogram } from '@opentelemetry/api' import { privacy } from './privacy' @@ -66,9 +64,9 @@ describe('Telemetry Service', () => { }) appContainer = await createTestApp(deps) - telemetryService = await deps.use('telemetry')! - aseRatesService = await deps.use('ratesService')! - internalRatesService = await deps.use('internalRatesService')! + telemetryService = await deps.use('telemetry') + aseRatesService = await deps.use('ratesService') + internalRatesService = await deps.use('internalRatesService') mockRatesApi(exchangeRatesUrl, (base) => { apiRequestCount++ @@ -80,6 +78,10 @@ describe('Telemetry Service', () => { await appContainer.shutdown() }) + test('telemetryService instance should be real implementation', () => { + expect(telemetryService instanceof TelemetryServiceImpl).toBe(true) + }) + it('should create a counter with source attribute for a new metric', () => { const name = 'test_counter' const amount = 1 @@ -441,29 +443,28 @@ describe('Telemetry Service', () => { }) }) describe('Telemetry Disabled', () => { - let deps: TelemetryServiceDependencies + let deps: IocContract + let appContainer: TestContainer + let telemetryService: TelemetryService - beforeEach(() => { - deps = { + beforeAll(async (): Promise => { + deps = initIocContainer({ + ...Config, enableTelemetry: false - } as TelemetryServiceDependencies + }) + appContainer = await createTestApp(deps) + telemetryService = await deps.use('telemetry')! }) - test('should return NoopTelemetryServiceImpl when enableTelemetry is false', () => { - const telemetryService = createTelemetryService(deps) - - expect(telemetryService).toBeInstanceOf(NoopTelemetryServiceImpl) + afterAll(async (): Promise => { + await appContainer.shutdown() }) - test('should return TelemetryServiceImpl when enableTelemetry is true', () => { - deps.enableTelemetry = true - const telemetryService = createTelemetryService(deps) - - expect(telemetryService).toBeInstanceOf(TelemetryServiceImpl) + test('telemetryService instance should be no-op implementation', () => { + expect(telemetryService instanceof NoopTelemetryServiceImpl).toBe(true) }) test('NoopTelemetryServiceImpl should not get meter ', () => { - const telemetryService = createTelemetryService(deps) telemetryService.recordHistogram('testhistogram', 1) telemetryService.incrementCounter('testcounter', 1) diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index 13d4eb6481..520e241964 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -40,7 +40,6 @@ export interface TelemetryServiceDependencies extends BaseService { internalRatesService: RatesService baseAssetCode: string baseScale: number - enableTelemetry: boolean } const METER_NAME = 'Rafiki' @@ -48,12 +47,13 @@ const METER_NAME = 'Rafiki' export function createTelemetryService( deps: TelemetryServiceDependencies ): TelemetryService { - if (!deps.enableTelemetry) { - return new NoopTelemetryServiceImpl(deps) - } return new TelemetryServiceImpl(deps) } +export function createNoopTelemetryService(): TelemetryService { + return new NoopTelemetryServiceImpl() +} + export class TelemetryServiceImpl implements TelemetryService { private instanceName: string private meterProvider?: MeterProvider @@ -223,16 +223,7 @@ export class TelemetryServiceImpl implements TelemetryService { /* eslint-disable @typescript-eslint/no-unused-vars */ export class NoopTelemetryServiceImpl implements TelemetryService { - private instanceName: string - private meterProvider?: MeterProvider - private internalRatesService: RatesService - private aseRatesService: RatesService - - constructor(private deps: TelemetryServiceDependencies) { - this.instanceName = deps.instanceName - this.internalRatesService = deps.internalRatesService - this.aseRatesService = deps.aseRatesService - } + constructor() {} public async shutdown(): Promise { // do nothing