Skip to content

feat(payments): add statsd logging to hot paths #18441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
julianpoy merged 1 commit into from
Feb 26, 2025
Merged

feat(payments): add statsd logging to hot paths #18441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion libs/google/src/lib/google.client.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Test } from '@nestjs/testing';
import { GeocodeResultFactory } from './factories';
import { GoogleClient } from './google.client';
import { MockGoogleClientConfigProvider } from './google.client.config';
import { MockStatsDProvider } from '@fxa/shared/metrics/statsd';

const mockJestFnGenerator = <T extends (...args: any[]) => any>() => {
return jest.fn<ReturnType<T>, Parameters<T>>();
Expand All @@ -26,7 +27,11 @@ describe('GoogleClient', () => {

beforeEach(async () => {
const module = await Test.createTestingModule({
providers: [GoogleClient, MockGoogleClientConfigProvider],
providers: [
GoogleClient,
MockGoogleClientConfigProvider,
MockStatsDProvider,
],
}).compile();

googleClient = module.get<GoogleClient>(GoogleClient);
Expand Down
13 changes: 11 additions & 2 deletions libs/google/src/lib/google.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,29 @@ import {
Client,
GeocodeResponseData,
} from '@googlemaps/google-maps-services-js';
import { Injectable } from '@nestjs/common';
import { Inject, Injectable } from '@nestjs/common';
import { GoogleClientConfig } from './google.client.config';
import {
CaptureTimingWithStatsD,
StatsDService,
type StatsD,
} from '@fxa/shared/metrics/statsd';

@Injectable()
export class GoogleClient {
private readonly google: Client;
constructor(private googleClientConfig: GoogleClientConfig) {
constructor(
private googleClientConfig: GoogleClientConfig,
@Inject(StatsDService) public statsd: StatsD
) {
this.google = new Client();
}

/**
* Retrieve Geocode Data for the specified address. For more information review
* https://developers.google.com/maps/documentation/geocoding/overview
*/
@CaptureTimingWithStatsD()
async geocode(address: string, countryCode: string) {
const response = await this.google.geocode({
params: {
Expand Down
8 changes: 7 additions & 1 deletion libs/google/src/lib/google.manager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,20 @@ import { GeocodeResultFactory } from './factories';
import { GoogleClient } from './google.client';
import { MockGoogleClientConfigProvider } from './google.client.config';
import { GoogleManager } from './google.manager';
import { MockStatsDProvider } from '@fxa/shared/metrics/statsd';

describe('GoogleManager', () => {
let googleClient: GoogleClient;
let googleManager: GoogleManager;

beforeEach(async () => {
const module = await Test.createTestingModule({
providers: [GoogleClient, GoogleManager, MockGoogleClientConfigProvider],
providers: [
GoogleClient,
GoogleManager,
MockGoogleClientConfigProvider,
MockStatsDProvider,
],
}).compile();

googleClient = module.get<GoogleClient>(GoogleClient);
Expand Down
26 changes: 14 additions & 12 deletions libs/payments/cart/src/lib/cart.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,12 +854,13 @@ describe('CartService', () => {

await cartService.finalizeProcessingCart(mockCart.id);

expect(checkoutService.postPaySteps).toHaveBeenCalledWith(
mockCart,
mockCart.version,
mockSubscription,
mockCart.uid
);
expect(checkoutService.postPaySteps).toHaveBeenCalledWith({
cart: mockCart,
version: mockCart.version,
subscription: mockSubscription,
uid: mockCart.uid,
paymentProvider: 'stripe',
});
});
});

Expand Down Expand Up @@ -1539,12 +1540,13 @@ describe('CartService', () => {
default_payment_method: mockPaymentMethod.id,
},
});
expect(checkoutService.postPaySteps).toHaveBeenCalledWith(
mockCart,
mockCart.version,
mockSubscription,
mockCart.uid
);
expect(checkoutService.postPaySteps).toHaveBeenCalledWith({
cart: mockCart,
version: mockCart.version,
subscription: mockSubscription,
uid: mockCart.uid,
paymentProvider: 'stripe',
});
expect(cartManager.finishErrorCart).not.toHaveBeenCalled();
});

Expand Down
30 changes: 16 additions & 14 deletions libs/payments/cart/src/lib/cart.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,6 @@ export class CartService {
uid?: string;
ip?: string;
}): Promise<ResultCart> {
// TODO:
// - Fetch information about interval, offering, experiments from CMS
// - Guess TaxAddress via maxmind client
// - Check if user is eligible to subscribe to plan, else throw error
// - Fetch stripeCustomerId if uid is passed and has a customer id
let accountCustomer;
if (args.uid) {
accountCustomer = await this.accountCustomerManager
Expand Down Expand Up @@ -443,12 +438,14 @@ export class CartService {
if (!subscription) {
throw new CartSubscriptionNotFoundError(cartId);
}
await this.checkoutService.postPaySteps(
await this.checkoutService.postPaySteps({
cart,
cart.version,
version: cart.version,
subscription,
cart.uid
);
uid: cart.uid,
paymentProvider:
this.subscriptionManager.getPaymentProvider(subscription),
});
});
}

Expand Down Expand Up @@ -603,7 +600,10 @@ export class CartService {
} else if (subscriptions.length) {
const firstListedSubscription = subscriptions[0];
// fetch payment method info
if (firstListedSubscription.collection_method === 'send_invoice') {
if (
this.subscriptionManager.getPaymentProvider(firstListedSubscription) ===
'paypal'
) {
// PayPal payment method collection
paymentInfo = {
type: 'external_paypal',
Expand Down Expand Up @@ -756,12 +756,14 @@ export class CartService {
const subscription = await this.subscriptionManager.retrieve(
cart.stripeSubscriptionId
);
await this.checkoutService.postPaySteps(
await this.checkoutService.postPaySteps({
cart,
cart.version,
version: cart.version,
subscription,
cart.uid
);
uid: cart.uid,
paymentProvider:
this.subscriptionManager.getPaymentProvider(subscription),
});
} else {
const promises: Promise<any>[] = [
this.finalizeCartWithError(cartId, CartErrorReasonId.Unknown),
Expand Down
60 changes: 36 additions & 24 deletions libs/payments/cart/src/lib/checkout.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,17 +411,20 @@ describe('CheckoutService', () => {
jest.spyOn(customerManager, 'setTaxId').mockResolvedValue();
jest.spyOn(profileClient, 'deleteCache').mockResolvedValue('test');
jest.spyOn(cartManager, 'finishCart').mockResolvedValue();
jest.spyOn(statsd, 'increment');
});

it('success', async () => {
const mockCart = ResultCartFactory();
const paymentProvider = 'stripe';

await checkoutService.postPaySteps(
mockCart,
mockCart.version,
mockSubscription,
mockUid
);
await checkoutService.postPaySteps({
cart: mockCart,
version: mockCart.version,
subscription: mockSubscription,
uid: mockUid,
paymentProvider,
});

expect(customerManager.setTaxId).toHaveBeenCalledWith(
mockSubscription.customer,
Expand All @@ -430,6 +433,11 @@ describe('CheckoutService', () => {

expect(privateMethod).toHaveBeenCalled();
expect(cartManager.finishCart).toHaveBeenCalled();
expect(statsd.increment).toHaveBeenCalledWith('subscription_success', {
payment_provider: paymentProvider,
offering_id: mockCart.offeringConfigId,
interval: mockCart.interval,
});
});

it('success - adds coupon code to subscription metadata if it exists', async () => {
Expand All @@ -444,17 +452,19 @@ describe('CheckoutService', () => {
},
})
);
const paymentProvider = 'stripe';

jest
.spyOn(subscriptionManager, 'update')
.mockResolvedValue(mockUpdatedSubscription);

await checkoutService.postPaySteps(
mockCart,
mockCart.version,
mockSubscription,
mockUid
);
await checkoutService.postPaySteps({
cart: mockCart,
version: mockCart.version,
subscription: mockSubscription,
uid: mockUid,
paymentProvider,
});

expect(customerManager.setTaxId).toHaveBeenCalledWith(
mockSubscription.customer,
Expand Down Expand Up @@ -598,12 +608,13 @@ describe('CheckoutService', () => {
});

it('calls postPaySteps', async () => {
expect(checkoutService.postPaySteps).toHaveBeenCalledWith(
mockCart,
mockPrePayStepsResult.version + 1,
mockSubscription,
mockCart.uid
);
expect(checkoutService.postPaySteps).toHaveBeenCalledWith({
cart: mockCart,
version: mockPrePayStepsResult.version + 1,
subscription: mockSubscription,
uid: mockCart.uid,
paymentProvider: 'stripe',
});
});
});

Expand Down Expand Up @@ -829,12 +840,13 @@ describe('CheckoutService', () => {
);
});
it('calls postPaySteps', async () => {
expect(checkoutService.postPaySteps).toHaveBeenCalledWith(
mockCart,
mockPrePayStepsResult.version + 1,
mockSubscription,
mockCart.uid
);
expect(checkoutService.postPaySteps).toHaveBeenCalledWith({
cart: mockCart,
version: mockPrePayStepsResult.version + 1,
subscription: mockSubscription,
uid: mockCart.uid,
paymentProvider: 'paypal',
});
});
});
describe('uncollectible', () => {
Expand Down
37 changes: 27 additions & 10 deletions libs/payments/cart/src/lib/checkout.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,12 +212,14 @@ export class CheckoutService {
};
}

async postPaySteps(
cart: ResultCart,
version: number,
subscription: StripeSubscription,
uid: string
) {
async postPaySteps(args: {
cart: ResultCart;
version: number;
subscription: StripeSubscription;
uid: string;
paymentProvider: 'stripe' | 'paypal';
}) {
const { cart, version, subscription, uid, paymentProvider } = args;
const { customer: customerId, currency } = subscription;

await this.customerManager.setTaxId(customerId, currency);
Expand All @@ -235,8 +237,11 @@ export class CheckoutService {
}
await this.cartManager.finishCart(cart.id, version, {});

// TODO: call sendFinishSetupEmailForStubAccount
console.log(cart.id, subscription.id);
this.statsd.increment('subscription_success', {
Copy link
Copy Markdown
Contributor

@StaberindeZA StaberindeZA Feb 25, 2025

Choose a reason for hiding this comment

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

To keep track of the amount, would another counter make sense?

It'd be great to have live dashboards where we can show successful payments by payment_provider, and also have a "Total Sales" for the day/week/month type dashboard.

Copy link
Copy Markdown
Member Author

@julianpoy julianpoy Feb 25, 2025

Choose a reason for hiding this comment

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

I think Stripe's dashboard does a good job of this and I'd rather push us towards capturing there, but I'm not opposed to adding an additional counter for subscription_total_amount that increments by the dollar amount of the sub.

Copy link
Copy Markdown
Contributor

@StaberindeZA StaberindeZA Feb 25, 2025

Choose a reason for hiding this comment

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

I think it could be cool to be able to filter it by payment_provider and offering/interval, which is something Stripe doesn't currently make super easy afaik.

Copy link
Copy Markdown
Contributor

@StaberindeZA StaberindeZA Feb 25, 2025

Choose a reason for hiding this comment

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

I guess currency would need to be included as well. Hmm. If this ends up being too much effort for what its worth, happy to leave it for another time , if at all.

Copy link
Copy Markdown
Member Author

@julianpoy julianpoy Feb 25, 2025

Choose a reason for hiding this comment

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

Ah, good point about currency -- the amount is not standard since it's currency-dependent. I'd like to push this for now if that's alright

payment_provider: paymentProvider,
offering_id: cart.offeringConfigId,
interval: cart.interval,
});
}

async payWithStripe(
Expand Down Expand Up @@ -319,7 +324,13 @@ export class CheckoutService {
{ cartId: cart.id }
);
}
await this.postPaySteps(cart, updatedVersion, subscription, uid);
await this.postPaySteps({
cart,
version: updatedVersion,
subscription,
uid,
paymentProvider: 'stripe',
});
} else {
throw new CheckoutPaymentError(
`Expected payment intent status to be one of [requires_action, succeeded], instead found: ${paymentIntent.status}`
Expand Down Expand Up @@ -409,7 +420,13 @@ export class CheckoutService {
latestInvoice
);
if (['paid', 'open'].includes(processedInvoice.status ?? '')) {
await this.postPaySteps(cart, updatedVersion, subscription, uid);
await this.postPaySteps({
cart,
version: updatedVersion,
subscription,
uid,
paymentProvider: 'paypal',
});
} else {
throw new CheckoutPaymentError(
`Expected processed invoice status to be one of [paid, open], instead found: ${processedInvoice.status}`
Expand Down
13 changes: 11 additions & 2 deletions libs/payments/content-server/src/lib/content-server.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,26 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import { Injectable } from '@nestjs/common';
import { Inject, Injectable } from '@nestjs/common';
import { ContentServerClientConfig } from './content-server.config';
import { MetricsFlow } from './content-server.types';
import {
CaptureTimingWithStatsD,
StatsDService,
type StatsD,
} from '@fxa/shared/metrics/statsd';

@Injectable()
export class ContentServerClient {
private readonly url: string;
constructor(private contentServerClientConfig: ContentServerClientConfig) {
constructor(
private contentServerClientConfig: ContentServerClientConfig,
@Inject(StatsDService) public statsd: StatsD
) {
this.url = this.contentServerClientConfig.url || 'http://localhost:3030';
}

@CaptureTimingWithStatsD()
public async getMetricsFlow() {
const response = await fetch(`${this.url}/metrics-flow`, { method: 'GET' });
if (!response.ok) {
Expand Down
8 changes: 7 additions & 1 deletion libs/payments/customer/src/lib/customer.manager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,20 @@ import {
} from '@fxa/payments/stripe';
import { TaxAddressFactory } from './factories/tax-address.factory';
import { CustomerManager } from './customer.manager';
import { MockStatsDProvider } from '@fxa/shared/metrics/statsd';

describe('CustomerManager', () => {
let customerManager: CustomerManager;
let stripeClient: StripeClient;

beforeEach(async () => {
const module = await Test.createTestingModule({
providers: [MockStripeConfigProvider, StripeClient, CustomerManager],
providers: [
MockStripeConfigProvider,
StripeClient,
CustomerManager,
MockStatsDProvider,
],
}).compile();

customerManager = module.get(CustomerManager);
Expand Down
Loading