Skip to content

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

Merged
julianpoy merged 1 commit intomainfrom
FXA-10618
Feb 26, 2025
Merged

feat(payments): add statsd logging to hot paths#18441
julianpoy merged 1 commit intomainfrom
FXA-10618

Conversation

@julianpoy
Copy link
Copy Markdown
Member

@julianpoy julianpoy commented Feb 24, 2025

Because

  • We want to log interactions with various services to statsd

This pull request

  • Adds a statsd decorator, as well as a few specific calls to statsd after important events

Issue that this pull request solves

Closes FXA-10618
Closes FXA-8304

@julianpoy julianpoy force-pushed the FXA-10618 branch 6 times, most recently from 734de3b to e3e19db Compare February 24, 2025 22:35
@julianpoy julianpoy marked this pull request as ready for review February 25, 2025 07:13
@julianpoy julianpoy requested a review from a team as a code owner February 25, 2025 07:13
Comment thread libs/payments/cart/src/lib/checkout.service.ts Outdated
Copy link
Copy Markdown
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

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

There are a few of payments-nexts external dependencies missing that we should gather request count and timings on as well.

  • Content Server (libs/payments/content-server/**) - Used to fetch flow_id at start of checkout
  • Google Maps services (libs/google/**) - Used in Tax Location picker

Comment thread libs/shared/metrics/statsd/src/lib/statsd.decorator.ts
}
await this.cartManager.finishCart(cart.id, version, {});

this.statsd.increment('subscription_success', {
Copy link
Copy Markdown
Contributor

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

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

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

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

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

Because:

- We want to log interactions with various services to statsd

This commit:

- Adds a statsd decorator, as well as a few specific calls to statsd
  after important events

Closes FXA-10618
Closes FXA-8304
Copy link
Copy Markdown
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

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

r+. I'm seeing them locally. Thank you!

@julianpoy julianpoy merged commit 7b78484 into main Feb 26, 2025
@julianpoy julianpoy deleted the FXA-10618 branch February 26, 2025 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants