From 362232d4b3681c5fc50be5ce5e09235af7ec03d3 Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Fri, 12 Jul 2024 10:59:26 -0400 Subject: [PATCH 1/9] refactor(backend): abstract telemetry metric recording --- .../payment/outgoing/lifecycle.ts | 21 ++--- .../backend/src/telemetry/service.test.ts | 79 ++++++++++++++----- packages/backend/src/telemetry/service.ts | 76 ++++++++++-------- .../src/telemetry/transaction-amount.test.ts | 38 ++++----- .../src/telemetry/transaction-amount.ts | 10 ++- packages/backend/src/tests/telemetry.ts | 32 +++----- 6 files changed, 143 insertions(+), 113 deletions(-) diff --git a/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts b/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts index 150aec5409..a825bd6515 100644 --- a/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts +++ b/packages/backend/src/open_payments/payment/outgoing/lifecycle.ts @@ -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' + }) } await handleCompleted(deps, payment) diff --git a/packages/backend/src/telemetry/service.test.ts b/packages/backend/src/telemetry/service.test.ts index 52ef956f67..b2ba521510 100644 --- a/packages/backend/src/telemetry/service.test.ts +++ b/packages/backend/src/telemetry/service.test.ts @@ -6,6 +6,7 @@ import { ConvertError, RatesService } from '../rates/service' import { TestContainer, createTestApp } from '../tests/app' import { mockCounter, mockHistogram } from '../tests/telemetry' import { TelemetryService } from './service' +import { Counter, Histogram } from '@opentelemetry/api' jest.mock('@opentelemetry/api', () => ({ ...jest.requireActual('@opentelemetry/api'), @@ -53,35 +54,71 @@ describe('TelemetryServiceImpl', () => { await appContainer.shutdown() }) - it('should create a counter when getOrCreate is called for a new metric', () => { - const counter = telemetryService.getOrCreateMetric('testMetric') - expect(counter).toBe(mockCounter) - }) + 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) - it('should return an existing counter when getOrCreate is called for an existing metric', () => { - const existingCounter = telemetryService.getOrCreateMetric('existingMetric') - const retrievedCounter = - telemetryService.getOrCreateMetric('existingMetric') - expect(retrievedCounter).toBe(existingCounter) + expect(mockCounter.add).toHaveBeenCalledWith( + amount, + expect.objectContaining({ + ...attributes, + source: expect.any(String) + }) + ) }) - it('should create a histogram when getOrCreateHistogramMetric is called for a new metric', () => { - const histogram = telemetryService.getOrCreateHistogramMetric('testMetric') - expect(histogram).toBe(mockHistogram) + 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) + + expect(mockHistogram.record).toHaveBeenCalledWith( + amount, + expect.objectContaining({ + ...attributes, + source: expect.any(String) + }) + ) }) - it('should return an existing histogram when getOrCreateHistogramMetric is called for an existing metric', () => { - const existingHistogram = - telemetryService.getOrCreateHistogramMetric('existingMetric') - const retrievedHistogram = - telemetryService.getOrCreateHistogramMetric('existingMetric') - expect(retrievedHistogram).toBe(existingHistogram) + 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) + + // Reflect to access private class variable + const counters: Map = Reflect.get(telemetryService, 'counters') + + expect(counters.size).toBe(1) + expect(counters.has(name)).toBe(true) + + const counter: Counter = counters.get(name) + expect(counter.add).toHaveBeenCalledTimes(2) }) - it('should return the instance name when calling getInstanceName', () => { - const serviceName = telemetryService.getInstanceName() + 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) + + // Reflect to access private class variable + const histograms: Map = Reflect.get( + telemetryService, + 'histograms' + ) + + expect(histograms.size).toBe(1) + expect(histograms.has(name)).toBe(true) - expect(serviceName).toBe('Rafiki') + const histogram: Histogram = histograms.get(name) + expect(histogram.record).toHaveBeenCalledTimes(2) }) describe('conversion', () => { diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index de6a5779fb..dae49a84f2 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -11,10 +11,17 @@ import { ConvertOptions } from '../rates/util' import { BaseService } from '../shared/baseService' export interface TelemetryService { - shutdown(): void - getOrCreateMetric(name: string, options?: MetricOptions): Counter - getOrCreateHistogramMetric(name: string, options?: MetricOptions): Histogram - getInstanceName(): string | undefined + shutdown(): Promise + incrementCounter( + name: string, + amount: number, + attributes?: Record + ): void + recordHistogram( + name: string, + value: number, + attributes?: Record + ): void getBaseAssetCode(): string getBaseScale(): number convertAmount( @@ -77,6 +84,7 @@ class TelemetryServiceImpl implements TelemetryService { exportIntervalMillis: deps.exportIntervalMillis ?? 15000 }) + // TODO: update deprecated method this.meterProvider?.addMetricReader(metricReader) }) @@ -87,39 +95,49 @@ class TelemetryServiceImpl implements TelemetryService { await this.meterProvider?.shutdown() } - private createHistogram(name: string, options: MetricOptions | undefined) { - const histogram = metrics - .getMeter(METER_NAME) - .createHistogram(name, options) - this.histograms.set(name, histogram) - return histogram + private getOrCreateCounter(name: string, options?: MetricOptions): Counter { + let counter = this.counters.get(name) + if (!counter) { + counter = metrics.getMeter(METER_NAME).createCounter(name, options) + this.counters.set(name, counter) + } + return counter } - public getOrCreateHistogramMetric( + + private getOrCreateHistogram( name: string, options?: MetricOptions ): Histogram { - const existing = this.histograms.get(name) - if (existing) { - return existing + let histogram = this.histograms.get(name) + if (!histogram) { + histogram = metrics.getMeter(METER_NAME).createHistogram(name, options) + this.histograms.set(name, histogram) } - return this.createHistogram(name, options) + return histogram } - private createCounter( + public incrementCounter( name: string, - options: MetricOptions | undefined - ): Counter { - const counter = metrics.getMeter(METER_NAME).createCounter(name, options) - this.counters.set(name, counter) - return counter + amount: number, + attributes: Record = {} + ): void { + const counter = this.getOrCreateCounter(name) + counter.add(amount, { + source: this.instanceName, + ...attributes + }) } - public getOrCreateMetric(name: string, options?: MetricOptions): Counter { - const existing = this.counters.get(name) - if (existing) { - return existing - } - return this.createCounter(name, options) + public recordHistogram( + name: string, + value: number, + attributes: Record = {} + ): void { + const histogram = this.getOrCreateHistogram(name) + histogram.record(value, { + source: this.instanceName, + ...attributes + }) } public async convertAmount( @@ -151,10 +169,6 @@ class TelemetryServiceImpl implements TelemetryService { return converted } - public getInstanceName(): string | undefined { - return this.instanceName - } - getBaseAssetCode(): string { return this.deps.baseAssetCode } diff --git a/packages/backend/src/telemetry/transaction-amount.test.ts b/packages/backend/src/telemetry/transaction-amount.test.ts index cb4d73e357..940eaa21f0 100644 --- a/packages/backend/src/telemetry/transaction-amount.test.ts +++ b/packages/backend/src/telemetry/transaction-amount.test.ts @@ -19,13 +19,7 @@ describe('Telemetry Amount Collection', function () { Promise.resolve(ConvertError.InvalidDestinationPrice) ) - const addSpy = jest.spyOn( - telemetryService.getOrCreateMetric('transactions_amount', { - description: 'Amount sent through the network', - valueType: ValueType.DOUBLE - }), - 'add' - ) + const incrementCounterSpy = jest.spyOn(telemetryService, 'incrementCounter') await collectTelemetryAmount(telemetryService, mockLogger, { amount: 100n, @@ -33,7 +27,7 @@ describe('Telemetry Amount Collection', function () { }) expect(convertSpy).toHaveBeenCalled() - expect(addSpy).not.toHaveBeenCalled() + expect(incrementCounterSpy).not.toHaveBeenCalled() }) it('should handle invalid amount by not collecting telemetry', async () => { const convertSpy = jest @@ -54,13 +48,7 @@ describe('Telemetry Amount Collection', function () { const convertSpy = jest .spyOn(telemetryService, 'convertAmount') .mockImplementation(() => Promise.resolve(10000n)) - const addSpy = jest.spyOn( - telemetryService.getOrCreateMetric('transactions_amount', { - description: 'Amount sent through the network', - valueType: ValueType.DOUBLE - }), - 'add' - ) + const incrementCounterSpy = jest.spyOn(telemetryService, 'incrementCounter') jest.spyOn(privacy, 'applyPrivacy').mockReturnValue(12000) await collectTelemetryAmount(telemetryService, mockLogger, { @@ -69,7 +57,11 @@ describe('Telemetry Amount Collection', function () { }) expect(convertSpy).toHaveBeenCalled() - expect(addSpy).toHaveBeenCalledWith(12000) + expect(incrementCounterSpy).toHaveBeenCalledWith( + 'transactions_amount', + 12000, + { description: 'Amount sent through the network', valueType: 1 } + ) }) it('should apply privacy to the collected telemetry', async () => { @@ -79,13 +71,7 @@ describe('Telemetry Amount Collection', function () { const privacySpy = jest .spyOn(privacy, 'applyPrivacy') .mockReturnValue(12000) - const addSpy = jest.spyOn( - telemetryService.getOrCreateMetric('transactions_amount', { - description: 'Amount sent through the network', - valueType: ValueType.DOUBLE - }), - 'add' - ) + const incrementCounterSpy = jest.spyOn(telemetryService, 'incrementCounter') await collectTelemetryAmount(telemetryService, mockLogger, { amount: 100n, @@ -94,6 +80,10 @@ describe('Telemetry Amount Collection', function () { expect(convertSpy).toHaveBeenCalled() expect(privacySpy).toHaveBeenCalledWith(Number(10000n)) - expect(addSpy).toHaveBeenCalledWith(12000) + expect(incrementCounterSpy).toHaveBeenCalledWith( + 'transactions_amount', + 12000, + { description: 'Amount sent through the network', valueType: 1 } + ) }) }) diff --git a/packages/backend/src/telemetry/transaction-amount.ts b/packages/backend/src/telemetry/transaction-amount.ts index c07700ca8f..f553a6c3ae 100644 --- a/packages/backend/src/telemetry/transaction-amount.ts +++ b/packages/backend/src/telemetry/transaction-amount.ts @@ -28,12 +28,14 @@ export async function collectTelemetryAmount( return } - telemetryService - .getOrCreateMetric('transactions_amount', { + telemetryService.incrementCounter( + 'transactions_amount', + privacy.applyPrivacy(Number(converted)), + { description: 'Amount sent through the network', valueType: ValueType.DOUBLE - }) - .add(privacy.applyPrivacy(Number(converted))) + } + ) } catch (e) { logger.error(e, `Unable to collect telemetry`) } diff --git a/packages/backend/src/tests/telemetry.ts b/packages/backend/src/tests/telemetry.ts index d9678bb7bb..90caf63127 100644 --- a/packages/backend/src/tests/telemetry.ts +++ b/packages/backend/src/tests/telemetry.ts @@ -1,9 +1,4 @@ -import { - Attributes, - Counter, - Histogram, - MetricOptions -} from '@opentelemetry/api' +import { Counter, Histogram } from '@opentelemetry/api' import { TelemetryService } from '../telemetry/service' import { ConvertError, Rates, RatesService } from '../rates/service' import { ConvertOptions } from '../rates/util' @@ -36,22 +31,21 @@ export class MockRatesService implements RatesService { export class MockTelemetryService implements TelemetryService { public aseRatesService = new MockRatesService() public internalRatesService = new MockRatesService() - public getOrCreateMetric( - _name: string, - _options?: MetricOptions | undefined - ): Counter { - return mockCounter - } - public getOrCreateHistogramMetric( - _name: string, - _options?: MetricOptions | undefined - ): Histogram { - return mockHistogram - } + + incrementCounter( + name: string, + amount?: number, + attributes?: Record + ): void {} + recordHistogram( + name: string, + value: number, + attributes?: Record + ): void {} public getInstanceName(): string | undefined { return 'serviceName' } - public shutdown(): void {} + public async shutdown(): Promise {} public async convertAmount( _convertOptions: Omit From 226be15adc2a8332a33ab6ad59387940d02fb60a Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Fri, 12 Jul 2024 11:57:30 -0400 Subject: [PATCH 2/9] fix(backend): deprecated opentelemetry method opentelemetry deprecation message: "This method will be removed in SDK 2.0. Please use MeterProviderOptions.readers via the MeterProvider constructor instead" --- packages/backend/src/telemetry/service.ts | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index dae49a84f2..ffa2c4d825 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -74,18 +74,16 @@ class TelemetryServiceImpl implements TelemetryService { resource: new Resource({ 'service.name': SERVICE_NAME }) }) - deps.collectorUrls.forEach((url) => { - const metricExporter = new OTLPMetricExporter({ - url: url - }) - - const metricReader = new PeriodicExportingMetricReader({ - exporter: metricExporter, - exportIntervalMillis: deps.exportIntervalMillis ?? 15000 + this.meterProvider = new MeterProvider({ + resource: new Resource({ 'service.name': SERVICE_NAME }), + readers: deps.collectorUrls.map((url) => { + return new PeriodicExportingMetricReader({ + exporter: new OTLPMetricExporter({ + url + }), + exportIntervalMillis: deps.exportIntervalMillis ?? 15000 + }) }) - - // TODO: update deprecated method - this.meterProvider?.addMetricReader(metricReader) }) metrics.setGlobalMeterProvider(this.meterProvider) From c04e2a5a712901e4392648e310e01329cf7832db Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Mon, 15 Jul 2024 14:12:21 -0400 Subject: [PATCH 3/9] refactor(backend): move amount conversion/privacy to telemetry service --- .../connector/core/middleware/telemetry.ts | 27 +++--- .../core/test/middleware/telemetry.test.ts | 36 ++++---- .../backend/src/telemetry/service.test.ts | 89 +++++++++++++++++-- packages/backend/src/telemetry/service.ts | 34 +++++-- .../src/telemetry/transaction-amount.test.ts | 89 ------------------- .../src/telemetry/transaction-amount.ts | 42 --------- packages/backend/src/tests/telemetry.ts | 5 ++ 7 files changed, 149 insertions(+), 173 deletions(-) delete mode 100644 packages/backend/src/telemetry/transaction-amount.test.ts delete mode 100644 packages/backend/src/telemetry/transaction-amount.ts diff --git a/packages/backend/src/payment-method/ilp/connector/core/middleware/telemetry.ts b/packages/backend/src/payment-method/ilp/connector/core/middleware/telemetry.ts index dd3682284b..409e82163e 100644 --- a/packages/backend/src/payment-method/ilp/connector/core/middleware/telemetry.ts +++ b/packages/backend/src/payment-method/ilp/connector/core/middleware/telemetry.ts @@ -1,4 +1,4 @@ -import { collectTelemetryAmount } from '../../../../../telemetry/transaction-amount' +import { ValueType } from '@opentelemetry/api' import { ILPContext, ILPMiddleware } from '../rafiki' export function createTelemetryMiddleware(): ILPMiddleware { @@ -7,16 +7,23 @@ export function createTelemetryMiddleware(): ILPMiddleware { next: () => Promise ): Promise => { await next() - if ( - services.telemetry && - Number(request.prepare.amount) && - response.fulfill - ) { + + const sourceAmount = BigInt(request.prepare.amount) + + if (services.telemetry && sourceAmount && response.fulfill) { const { code, scale } = accounts.outgoing.asset - collectTelemetryAmount(services.telemetry, services.logger, { - amount: BigInt(request.prepare.amount), - asset: { code: code, scale: scale } - }) + + await services.telemetry.incrementCounterWithAmount( + 'transactions_amount', + { + sourceAmount, + sourceAsset: { code: code, scale: scale } + }, + { + description: 'Amount sent through the network', + valueType: ValueType.DOUBLE + } + ) } } } diff --git a/packages/backend/src/payment-method/ilp/connector/core/test/middleware/telemetry.test.ts b/packages/backend/src/payment-method/ilp/connector/core/test/middleware/telemetry.test.ts index 05b4773747..86574ce29f 100644 --- a/packages/backend/src/payment-method/ilp/connector/core/test/middleware/telemetry.test.ts +++ b/packages/backend/src/payment-method/ilp/connector/core/test/middleware/telemetry.test.ts @@ -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' }) @@ -35,7 +33,6 @@ const ctx = createILPContext({ } as IlpResponse }) -jest.mock('../../../../../../telemetry/transaction-amount') const middleware = createTelemetryMiddleware() const next = jest.fn().mockImplementation(() => Promise.resolve()) @@ -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 incrementCounterWithAmountSpy = jest + .spyOn(services.telemetry, 'incrementCounterWithAmount') .mockImplementation(() => Promise.resolve()) await middleware( { ...ctx, services: { ...ctx.services, telemetry: undefined } }, next ) - expect(collectAmountSpy).not.toHaveBeenCalled() + + expect(incrementCounterWithAmountSpy).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 incrementCounterWithAmountSpy = jest + .spyOn(services.telemetry, 'incrementCounterWithAmount') + .mockImplementation(() => Promise.resolve()) await middleware( { ...ctx, response: { fulfill: undefined } as IlpResponse }, next ) - expect(collectAmountSpy).not.toHaveBeenCalled() + expect(incrementCounterWithAmountSpy).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 incrementCounterWithAmountSpy = jest + .spyOn(services.telemetry, 'incrementCounterWithAmount') + .mockImplementation(() => Promise.resolve()) await middleware( { @@ -85,19 +87,19 @@ describe('Telemetry Middleware', function () { next ) - expect(collectAmountSpy).not.toHaveBeenCalled() + expect(incrementCounterWithAmountSpy).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 incrementCounterWithAmountSpy = jest + .spyOn(services.telemetry, 'incrementCounterWithAmount') .mockImplementation(() => { expect(nextCalled).toBe(true) return Promise.resolve() @@ -105,7 +107,7 @@ describe('Telemetry Middleware', function () { await middleware(ctx, next) - expect(collectAmountSpy).toHaveBeenCalled() + expect(incrementCounterWithAmountSpy).toHaveBeenCalled() expect(next).toHaveBeenCalled() }) }) diff --git a/packages/backend/src/telemetry/service.test.ts b/packages/backend/src/telemetry/service.test.ts index b2ba521510..f5fe863c92 100644 --- a/packages/backend/src/telemetry/service.test.ts +++ b/packages/backend/src/telemetry/service.test.ts @@ -7,6 +7,7 @@ import { TestContainer, createTestApp } from '../tests/app' import { mockCounter, mockHistogram } from '../tests/telemetry' import { TelemetryService } from './service' import { Counter, Histogram } from '@opentelemetry/api' +import { privacy } from './privacy' jest.mock('@opentelemetry/api', () => ({ ...jest.requireActual('@opentelemetry/api'), @@ -92,8 +93,7 @@ describe('TelemetryServiceImpl', () => { telemetryService.incrementCounter(name, 1) telemetryService.incrementCounter(name, 1) - // Reflect to access private class variable - const counters: Map = Reflect.get(telemetryService, 'counters') + const counters: Map = (telemetryService as any).counters expect(counters.size).toBe(1) expect(counters.has(name)).toBe(true) @@ -121,43 +121,114 @@ describe('TelemetryServiceImpl', () => { expect(histogram.record).toHaveBeenCalledTimes(2) }) - describe('conversion', () => { + describe('incrementCounterWithAmount', () => { 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)) - const converted = await telemetryService.convertAmount({ + await telemetryService.incrementCounterWithAmount('test_counter', { sourceAmount: 100n, sourceAsset: { code: 'USD', scale: 2 } }) expect(aseConvertSpy).toHaveBeenCalled() expect(internalConvertSpy).toHaveBeenCalled() - expect(converted).toBe(10000n) }) 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') - const converted = await telemetryService.convertAmount({ + await telemetryService.incrementCounterWithAmount('test_counter', { sourceAmount: 100n, sourceAsset: { code: 'USD', scale: 2 } }) expect(aseConvertSpy).toHaveBeenCalled() expect(internalConvertSpy).not.toHaveBeenCalled() - expect(converted).toBe(500n) + }) + + it('should apply privacy', async () => { + const convertedAmount = 500n + + jest + .spyOn(telemetryService as any, 'convertAmount') + .mockImplementation(() => Promise.resolve(convertedAmount)) + const applyPrivacySpy = jest + .spyOn(privacy, 'applyPrivacy') + .mockReturnValue(123) + const incrementCounterSpy = jest.spyOn( + telemetryService, + 'incrementCounter' + ) + + const counterName = 'test_counter' + await telemetryService.incrementCounterWithAmount(counterName, { + sourceAmount: 100n, + sourceAsset: { code: 'USD', scale: 2 } + }) + + expect(applyPrivacySpy).toHaveBeenCalledWith(Number(convertedAmount)) + expect(incrementCounterSpy).toHaveBeenCalledWith( + counterName, + 123, + expect.any(Object) + ) + }) + + it('should not collect telemetry when conversion returns InvalidDestinationPrice', async () => { + const convertSpy = jest + .spyOn(telemetryService as any, 'convertAmount') + .mockImplementation(() => + Promise.resolve(ConvertError.InvalidDestinationPrice) + ) + + const incrementCounterSpy = jest.spyOn( + telemetryService, + 'incrementCounter' + ) + + await telemetryService.incrementCounterWithAmount('test_counter', { + sourceAmount: 100n, + sourceAsset: { code: 'USD', scale: 2 } + }) + + expect(convertSpy).toHaveBeenCalled() + expect(incrementCounterSpy).not.toHaveBeenCalled() + }) + + it('should collect telemetry when conversion is successful', async () => { + const convertSpy = jest + .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.incrementCounterWithAmount(counterName, { + sourceAmount: 100n, + sourceAsset: { code: 'USD', scale: 2 } + }) + + expect(convertSpy).toHaveBeenCalled() + expect(incrementCounterSpy).toHaveBeenCalledWith( + counterName, + obfuscatedAmount, + expect.any(Object) + ) }) }) }) diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index ffa2c4d825..545f860db2 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -6,9 +6,10 @@ import { PeriodicExportingMetricReader } from '@opentelemetry/sdk-metrics' -import { ConvertError, RatesService, isConvertError } from '../rates/service' +import { RatesService, isConvertError } from '../rates/service' import { ConvertOptions } from '../rates/util' import { BaseService } from '../shared/baseService' +import { privacy } from './privacy' export interface TelemetryService { shutdown(): Promise @@ -17,6 +18,11 @@ export interface TelemetryService { amount: number, attributes?: Record ): void + incrementCounterWithAmount( + name: string, + convertOptions: Pick, + attributes?: Record + ): Promise recordHistogram( name: string, value: number, @@ -24,9 +30,6 @@ export interface TelemetryService { ): void getBaseAssetCode(): string getBaseScale(): number - convertAmount( - convertOptions: Omit - ): Promise } interface TelemetryServiceDependencies extends BaseService { @@ -126,6 +129,25 @@ class TelemetryServiceImpl implements TelemetryService { }) } + public async incrementCounterWithAmount( + name: string, + amount: Pick, + attributes: Record = {} + ): Promise { + try { + const converted = await this.convertAmount(amount) + if (isConvertError(converted)) { + this.deps.logger.error(`Unable to convert amount: ${converted}`) + return + } + + const obfuscatedAmount = privacy.applyPrivacy(Number(converted)) + this.incrementCounter(name, obfuscatedAmount, attributes) + } catch (e) { + this.deps.logger.error(e, `Unable to collect telemetry`) + } + } + public recordHistogram( name: string, value: number, @@ -138,8 +160,8 @@ class TelemetryServiceImpl implements TelemetryService { }) } - public async convertAmount( - convertOptions: Omit + private async convertAmount( + convertOptions: Pick ) { const destinationAsset = { code: this.deps.baseAssetCode, diff --git a/packages/backend/src/telemetry/transaction-amount.test.ts b/packages/backend/src/telemetry/transaction-amount.test.ts deleted file mode 100644 index 940eaa21f0..0000000000 --- a/packages/backend/src/telemetry/transaction-amount.test.ts +++ /dev/null @@ -1,89 +0,0 @@ -import { ValueType } from '@opentelemetry/api' -import { ConvertError } from '../rates/service' -import { Asset } from '../rates/util' -import { MockTelemetryService } from '../tests/telemetry' -import { privacy } from './privacy' -import { collectTelemetryAmount } from './transaction-amount' -import { Logger } from 'pino' - -const telemetryService = new MockTelemetryService() -const mockLogger = { error: jest.fn() } as unknown as Logger - -const asset: Asset = { code: 'USD', scale: 2 } - -describe('Telemetry Amount Collection', function () { - it('should not collect telemetry when conversion returns InvalidDestinationPrice', async () => { - const convertSpy = jest - .spyOn(telemetryService, 'convertAmount') - .mockImplementation(() => - Promise.resolve(ConvertError.InvalidDestinationPrice) - ) - - const incrementCounterSpy = jest.spyOn(telemetryService, 'incrementCounter') - - await collectTelemetryAmount(telemetryService, mockLogger, { - amount: 100n, - asset - }) - - expect(convertSpy).toHaveBeenCalled() - expect(incrementCounterSpy).not.toHaveBeenCalled() - }) - it('should handle invalid amount by not collecting telemetry', async () => { - const convertSpy = jest - .spyOn(telemetryService, 'convertAmount') - .mockImplementation(() => - Promise.resolve(ConvertError.InvalidDestinationPrice) - ) - - await collectTelemetryAmount(telemetryService, mockLogger, { - amount: 0n, - asset - }) - - expect(convertSpy).not.toHaveBeenCalled() - }) - - it('should collect telemetry when conversion is successful', async () => { - const convertSpy = jest - .spyOn(telemetryService, 'convertAmount') - .mockImplementation(() => Promise.resolve(10000n)) - const incrementCounterSpy = jest.spyOn(telemetryService, 'incrementCounter') - jest.spyOn(privacy, 'applyPrivacy').mockReturnValue(12000) - - await collectTelemetryAmount(telemetryService, mockLogger, { - amount: 100n, - asset - }) - - expect(convertSpy).toHaveBeenCalled() - expect(incrementCounterSpy).toHaveBeenCalledWith( - 'transactions_amount', - 12000, - { description: 'Amount sent through the network', valueType: 1 } - ) - }) - - it('should apply privacy to the collected telemetry', async () => { - const convertSpy = jest - .spyOn(telemetryService, 'convertAmount') - .mockImplementation(() => Promise.resolve(10000n)) - const privacySpy = jest - .spyOn(privacy, 'applyPrivacy') - .mockReturnValue(12000) - const incrementCounterSpy = jest.spyOn(telemetryService, 'incrementCounter') - - await collectTelemetryAmount(telemetryService, mockLogger, { - amount: 100n, - asset - }) - - expect(convertSpy).toHaveBeenCalled() - expect(privacySpy).toHaveBeenCalledWith(Number(10000n)) - expect(incrementCounterSpy).toHaveBeenCalledWith( - 'transactions_amount', - 12000, - { description: 'Amount sent through the network', valueType: 1 } - ) - }) -}) diff --git a/packages/backend/src/telemetry/transaction-amount.ts b/packages/backend/src/telemetry/transaction-amount.ts deleted file mode 100644 index f553a6c3ae..0000000000 --- a/packages/backend/src/telemetry/transaction-amount.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { ValueType } from '@opentelemetry/api' -import { ConvertError } from '../rates/service' -import { Asset, ConvertOptions } from '../rates/util' -import { privacy } from './privacy' -import { TelemetryService } from './service' -import { Logger } from 'pino' - -export async function collectTelemetryAmount( - telemetryService: TelemetryService, - logger: Logger, - { amount, asset }: { amount: bigint; asset: Asset } -) { - if (!amount) { - return - } - - const convertOptions: Omit< - ConvertOptions, - 'exchangeRate' | 'destinationAsset' - > = { - sourceAmount: amount, - sourceAsset: { code: asset.code, scale: asset.scale } - } - - try { - const converted = await telemetryService.convertAmount(convertOptions) - if (converted === ConvertError.InvalidDestinationPrice) { - return - } - - telemetryService.incrementCounter( - 'transactions_amount', - privacy.applyPrivacy(Number(converted)), - { - description: 'Amount sent through the network', - valueType: ValueType.DOUBLE - } - ) - } catch (e) { - logger.error(e, `Unable to collect telemetry`) - } -} diff --git a/packages/backend/src/tests/telemetry.ts b/packages/backend/src/tests/telemetry.ts index 90caf63127..4210d96045 100644 --- a/packages/backend/src/tests/telemetry.ts +++ b/packages/backend/src/tests/telemetry.ts @@ -37,6 +37,11 @@ export class MockTelemetryService implements TelemetryService { amount?: number, attributes?: Record ): void {} + async incrementCounterWithAmount( + name: string, + convertOptions: Pick, + attributes?: Record + ): Promise {} recordHistogram( name: string, value: number, From 10fcdf8d8b6622e262d413648f8c01e46f58efb4 Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Mon, 15 Jul 2024 14:18:29 -0400 Subject: [PATCH 4/9] fix(backend): rm unused telemetry service methods --- packages/backend/src/telemetry/service.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index 545f860db2..2bd6932d72 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -28,8 +28,6 @@ export interface TelemetryService { value: number, attributes?: Record ): void - getBaseAssetCode(): string - getBaseScale(): number } interface TelemetryServiceDependencies extends BaseService { @@ -188,12 +186,4 @@ class TelemetryServiceImpl implements TelemetryService { } return converted } - - getBaseAssetCode(): string { - return this.deps.baseAssetCode - } - - getBaseScale(): number { - return this.deps.baseScale - } } From 6542616178e8736e639a8e1a3d4e57c6a61e6463 Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Mon, 15 Jul 2024 14:32:23 -0400 Subject: [PATCH 5/9] fix(backend): lint errors --- .../backend/src/telemetry/service.test.ts | 27 ++++++++++++------- packages/backend/src/tests/telemetry.ts | 18 +++---------- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/packages/backend/src/telemetry/service.test.ts b/packages/backend/src/telemetry/service.test.ts index f5fe863c92..4623141bb1 100644 --- a/packages/backend/src/telemetry/service.test.ts +++ b/packages/backend/src/telemetry/service.test.ts @@ -93,13 +93,15 @@ describe('TelemetryServiceImpl', () => { telemetryService.incrementCounter(name, 1) telemetryService.incrementCounter(name, 1) - 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) - const counter: 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', () => { @@ -108,17 +110,16 @@ describe('TelemetryServiceImpl', () => { telemetryService.recordHistogram(name, 1) telemetryService.recordHistogram(name, 1) - // Reflect to access private class variable - const histograms: Map = Reflect.get( - telemetryService, - '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) - const histogram: Histogram = histograms.get(name) - expect(histogram.record).toHaveBeenCalledTimes(2) + const histogram = histograms.get(name) + expect(histogram?.record).toHaveBeenCalledTimes(2) }) describe('incrementCounterWithAmount', () => { @@ -160,6 +161,8 @@ describe('TelemetryServiceImpl', () => { 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 @@ -186,6 +189,8 @@ describe('TelemetryServiceImpl', () => { 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) @@ -207,6 +212,8 @@ describe('TelemetryServiceImpl', () => { 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( diff --git a/packages/backend/src/tests/telemetry.ts b/packages/backend/src/tests/telemetry.ts index 4210d96045..1695f2fa80 100644 --- a/packages/backend/src/tests/telemetry.ts +++ b/packages/backend/src/tests/telemetry.ts @@ -32,21 +32,9 @@ export class MockTelemetryService implements TelemetryService { public aseRatesService = new MockRatesService() public internalRatesService = new MockRatesService() - incrementCounter( - name: string, - amount?: number, - attributes?: Record - ): void {} - async incrementCounterWithAmount( - name: string, - convertOptions: Pick, - attributes?: Record - ): Promise {} - recordHistogram( - name: string, - value: number, - attributes?: Record - ): void {} + incrementCounter(): void {} + async incrementCounterWithAmount(): Promise {} + recordHistogram(): void {} public getInstanceName(): string | undefined { return 'serviceName' } From a2fcc487a27bdde51bd3bd2e5a8b7306cb922295 Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Tue, 16 Jul 2024 10:47:30 -0400 Subject: [PATCH 6/9] Update packages/backend/src/telemetry/service.ts Co-authored-by: Sarah Jones --- packages/backend/src/telemetry/service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index 2bd6932d72..a546dc079a 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -20,7 +20,7 @@ export interface TelemetryService { ): void incrementCounterWithAmount( name: string, - convertOptions: Pick, + amount: Pick, attributes?: Record ): Promise recordHistogram( From 84d0c80674a1749f7c84ff4db3089eb30e2c872e Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Tue, 16 Jul 2024 12:13:16 -0400 Subject: [PATCH 7/9] feat(localenv): add ilp_pay_time_ms quartile viz grafana --- .../provisioning/dashboards/example.json | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/localenv/telemetry/grafana/provisioning/dashboards/example.json b/localenv/telemetry/grafana/provisioning/dashboards/example.json index 8826b6ecf8..1ab7c91b24 100644 --- a/localenv/telemetry/grafana/provisioning/dashboards/example.json +++ b/localenv/telemetry/grafana/provisioning/dashboards/example.json @@ -18,6 +18,7 @@ "editable": true, "fiscalYearStartMonth": 0, "graphTooltip": 0, + "id": 1, "links": [], "panels": [ { @@ -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", From 096be576b903f7acdd746a6e6c6fe11e200cde13 Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Wed, 17 Jul 2024 11:21:32 -0400 Subject: [PATCH 8/9] refactor(backend): rename tel service method, args --- .../connector/core/middleware/telemetry.ts | 13 ++-- .../core/test/middleware/telemetry.test.ts | 24 +++---- .../backend/src/telemetry/service.test.ts | 62 ++++++++++++------- packages/backend/src/telemetry/service.ts | 16 +++-- packages/backend/src/tests/telemetry.ts | 2 +- 5 files changed, 71 insertions(+), 46 deletions(-) diff --git a/packages/backend/src/payment-method/ilp/connector/core/middleware/telemetry.ts b/packages/backend/src/payment-method/ilp/connector/core/middleware/telemetry.ts index 409e82163e..8b28cf5cf6 100644 --- a/packages/backend/src/payment-method/ilp/connector/core/middleware/telemetry.ts +++ b/packages/backend/src/payment-method/ilp/connector/core/middleware/telemetry.ts @@ -8,16 +8,17 @@ export function createTelemetryMiddleware(): ILPMiddleware { ): Promise => { await next() - const sourceAmount = BigInt(request.prepare.amount) + const value = BigInt(request.prepare.amount) - if (services.telemetry && sourceAmount && response.fulfill) { - const { code, scale } = accounts.outgoing.asset + if (services.telemetry && value && response.fulfill) { + const { code: assetCode, scale: assetScale } = accounts.outgoing.asset - await services.telemetry.incrementCounterWithAmount( + await services.telemetry.incrementCounterWithTransactionAmount( 'transactions_amount', { - sourceAmount, - sourceAsset: { code: code, scale: scale } + value, + assetCode, + assetScale }, { description: 'Amount sent through the network', diff --git a/packages/backend/src/payment-method/ilp/connector/core/test/middleware/telemetry.test.ts b/packages/backend/src/payment-method/ilp/connector/core/test/middleware/telemetry.test.ts index 86574ce29f..e33e21afa9 100644 --- a/packages/backend/src/payment-method/ilp/connector/core/test/middleware/telemetry.test.ts +++ b/packages/backend/src/payment-method/ilp/connector/core/test/middleware/telemetry.test.ts @@ -44,8 +44,8 @@ beforeEach(async () => { describe('Telemetry Middleware', function () { it('should not gather telemetry if telemetry is not enabled (service is undefined)', async () => { - const incrementCounterWithAmountSpy = jest - .spyOn(services.telemetry, 'incrementCounterWithAmount') + const incrementCounterWithTransactionAmountSpy = jest + .spyOn(services.telemetry, 'incrementCounterWithTransactionAmount') .mockImplementation(() => Promise.resolve()) await middleware( @@ -53,13 +53,13 @@ describe('Telemetry Middleware', function () { next ) - expect(incrementCounterWithAmountSpy).not.toHaveBeenCalled() + expect(incrementCounterWithTransactionAmountSpy).not.toHaveBeenCalled() expect(next).toHaveBeenCalled() }) it('should not gather telemetry if response.fulfill undefined', async () => { - const incrementCounterWithAmountSpy = jest - .spyOn(services.telemetry, 'incrementCounterWithAmount') + const incrementCounterWithTransactionAmountSpy = jest + .spyOn(services.telemetry, 'incrementCounterWithTransactionAmount') .mockImplementation(() => Promise.resolve()) await middleware( @@ -67,13 +67,13 @@ describe('Telemetry Middleware', function () { next ) - expect(incrementCounterWithAmountSpy).not.toHaveBeenCalled() + expect(incrementCounterWithTransactionAmountSpy).not.toHaveBeenCalled() expect(next).toHaveBeenCalled() }) it('should not gather telemetry if request.prepare.amount is 0', async () => { - const incrementCounterWithAmountSpy = jest - .spyOn(services.telemetry, 'incrementCounterWithAmount') + const incrementCounterWithTransactionAmountSpy = jest + .spyOn(services.telemetry, 'incrementCounterWithTransactionAmount') .mockImplementation(() => Promise.resolve()) await middleware( @@ -87,7 +87,7 @@ describe('Telemetry Middleware', function () { next ) - expect(incrementCounterWithAmountSpy).not.toHaveBeenCalled() + expect(incrementCounterWithTransactionAmountSpy).not.toHaveBeenCalled() expect(next).toHaveBeenCalled() }) @@ -98,8 +98,8 @@ describe('Telemetry Middleware', function () { return Promise.resolve() }) - const incrementCounterWithAmountSpy = jest - .spyOn(services.telemetry, 'incrementCounterWithAmount') + const incrementCounterWithTransactionAmountSpy = jest + .spyOn(services.telemetry, 'incrementCounterWithTransactionAmount') .mockImplementation(() => { expect(nextCalled).toBe(true) return Promise.resolve() @@ -107,7 +107,7 @@ describe('Telemetry Middleware', function () { await middleware(ctx, next) - expect(incrementCounterWithAmountSpy).toHaveBeenCalled() + expect(incrementCounterWithTransactionAmountSpy).toHaveBeenCalled() expect(next).toHaveBeenCalled() }) }) diff --git a/packages/backend/src/telemetry/service.test.ts b/packages/backend/src/telemetry/service.test.ts index 4623141bb1..b741a17c9e 100644 --- a/packages/backend/src/telemetry/service.test.ts +++ b/packages/backend/src/telemetry/service.test.ts @@ -122,7 +122,7 @@ describe('TelemetryServiceImpl', () => { expect(histogram?.record).toHaveBeenCalledTimes(2) }) - describe('incrementCounterWithAmount', () => { + describe('incrementCounterWithTransactionAmount', () => { it('should try to convert using aseRatesService and fallback to internalRatesService', async () => { const aseConvertSpy = jest .spyOn(aseRatesService, 'convert') @@ -133,10 +133,14 @@ describe('TelemetryServiceImpl', () => { .spyOn(internalRatesService, 'convert') .mockImplementation(() => Promise.resolve(10000n)) - await telemetryService.incrementCounterWithAmount('test_counter', { - sourceAmount: 100n, - sourceAsset: { code: 'USD', scale: 2 } - }) + await telemetryService.incrementCounterWithTransactionAmount( + 'test_counter', + { + value: 100n, + assetCode: 'USD', + assetScale: 2 + } + ) expect(aseConvertSpy).toHaveBeenCalled() expect(internalConvertSpy).toHaveBeenCalled() @@ -148,10 +152,14 @@ describe('TelemetryServiceImpl', () => { .mockImplementation(() => Promise.resolve(500n)) const internalConvertSpy = jest.spyOn(internalRatesService, 'convert') - await telemetryService.incrementCounterWithAmount('test_counter', { - sourceAmount: 100n, - sourceAsset: { code: 'USD', scale: 2 } - }) + await telemetryService.incrementCounterWithTransactionAmount( + 'test_counter', + { + value: 100n, + assetCode: 'USD', + assetScale: 2 + } + ) expect(aseConvertSpy).toHaveBeenCalled() expect(internalConvertSpy).not.toHaveBeenCalled() @@ -174,10 +182,14 @@ describe('TelemetryServiceImpl', () => { ) const counterName = 'test_counter' - await telemetryService.incrementCounterWithAmount(counterName, { - sourceAmount: 100n, - sourceAsset: { code: 'USD', scale: 2 } - }) + await telemetryService.incrementCounterWithTransactionAmount( + counterName, + { + value: 100n, + assetCode: 'USD', + assetScale: 2 + } + ) expect(applyPrivacySpy).toHaveBeenCalledWith(Number(convertedAmount)) expect(incrementCounterSpy).toHaveBeenCalledWith( @@ -201,10 +213,14 @@ describe('TelemetryServiceImpl', () => { 'incrementCounter' ) - await telemetryService.incrementCounterWithAmount('test_counter', { - sourceAmount: 100n, - sourceAsset: { code: 'USD', scale: 2 } - }) + await telemetryService.incrementCounterWithTransactionAmount( + 'test_counter', + { + value: 100n, + assetCode: 'USD', + assetScale: 2 + } + ) expect(convertSpy).toHaveBeenCalled() expect(incrementCounterSpy).not.toHaveBeenCalled() @@ -225,10 +241,14 @@ describe('TelemetryServiceImpl', () => { const counterName = 'test_counter' - await telemetryService.incrementCounterWithAmount(counterName, { - sourceAmount: 100n, - sourceAsset: { code: 'USD', scale: 2 } - }) + await telemetryService.incrementCounterWithTransactionAmount( + counterName, + { + value: 100n, + assetCode: 'USD', + assetScale: 2 + } + ) expect(convertSpy).toHaveBeenCalled() expect(incrementCounterSpy).toHaveBeenCalledWith( diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index a546dc079a..45df6ca9cb 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -15,12 +15,12 @@ export interface TelemetryService { shutdown(): Promise incrementCounter( name: string, - amount: number, + value: number, attributes?: Record ): void - incrementCounterWithAmount( + incrementCounterWithTransactionAmount( name: string, - amount: Pick, + amount: { value: bigint; assetCode: string; assetScale: number }, attributes?: Record ): Promise recordHistogram( @@ -127,13 +127,17 @@ class TelemetryServiceImpl implements TelemetryService { }) } - public async incrementCounterWithAmount( + public async incrementCounterWithTransactionAmount( name: string, - amount: Pick, + amount: { value: bigint; assetCode: string; assetScale: number }, attributes: Record = {} ): Promise { + const { value, assetCode, assetScale } = amount try { - const converted = await this.convertAmount(amount) + const converted = await this.convertAmount({ + sourceAmount: value, + sourceAsset: { code: assetCode, scale: assetScale } + }) if (isConvertError(converted)) { this.deps.logger.error(`Unable to convert amount: ${converted}`) return diff --git a/packages/backend/src/tests/telemetry.ts b/packages/backend/src/tests/telemetry.ts index 1695f2fa80..d682d40f96 100644 --- a/packages/backend/src/tests/telemetry.ts +++ b/packages/backend/src/tests/telemetry.ts @@ -33,7 +33,7 @@ export class MockTelemetryService implements TelemetryService { public internalRatesService = new MockRatesService() incrementCounter(): void {} - async incrementCounterWithAmount(): Promise {} + async incrementCounterWithTransactionAmount(): Promise {} recordHistogram(): void {} public getInstanceName(): string | undefined { return 'serviceName' From a9feb4357e4c9dfa13abb23fe97a3ce634287724 Mon Sep 17 00:00:00 2001 From: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Date: Wed, 17 Jul 2024 11:25:45 -0400 Subject: [PATCH 9/9] chore(backend): rm unused/redundant telemetry code --- packages/backend/src/telemetry/service.ts | 4 ---- packages/backend/src/tests/telemetry.ts | 8 -------- 2 files changed, 12 deletions(-) diff --git a/packages/backend/src/telemetry/service.ts b/packages/backend/src/telemetry/service.ts index 45df6ca9cb..31372750d2 100644 --- a/packages/backend/src/telemetry/service.ts +++ b/packages/backend/src/telemetry/service.ts @@ -71,10 +71,6 @@ class TelemetryServiceImpl implements TelemetryService { return } - this.meterProvider = new MeterProvider({ - resource: new Resource({ 'service.name': SERVICE_NAME }) - }) - this.meterProvider = new MeterProvider({ resource: new Resource({ 'service.name': SERVICE_NAME }), readers: deps.collectorUrls.map((url) => { diff --git a/packages/backend/src/tests/telemetry.ts b/packages/backend/src/tests/telemetry.ts index d682d40f96..ec5d5650fe 100644 --- a/packages/backend/src/tests/telemetry.ts +++ b/packages/backend/src/tests/telemetry.ts @@ -49,12 +49,4 @@ export class MockTelemetryService implements TelemetryService { } return Promise.resolve(converted) } - - public getBaseAssetCode(): string { - return 'USD' - } - - public getBaseScale(): number { - return 4 - } }