Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(backend): performance and caching poc #2954

Draft
wants to merge 48 commits into
base: main
Choose a base branch
from
Draft

Conversation

koekiebox
Copy link
Collaborator

Changes proposed in this pull request

Context

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

koekiebox and others added 30 commits August 26, 2024 17:17
* feat(backend): add create quote service metrics

* feat: add getQuote traces
# Conflicts:
#	localenv/cloud-nine-wallet/dbinit.sql
# Conflicts:
#	localenv/telemetry/grafana/provisioning/dashboards/example.json
#	test/performance/scripts/create-outgoing-payments.js
# Conflicts:
#	test/performance/scripts/create-outgoing-payments.js
@koekiebox koekiebox self-assigned this Sep 9, 2024
@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Sep 9, 2024
Copy link

netlify bot commented Sep 9, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 282a717
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66dee8cd5e24b30008963d12

const incomingPayment = await IncomingPayment.query(deps.knex).get(options)
await deps.assetService.setOn(incomingPayment)
await deps.walletAddressService.setOn(incomingPayment)

Copy link
Contributor

Choose a reason for hiding this comment

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

just for documentation's sake (dont necessarily need to do it here), Radu had a good suggestion for how to do this better.

We could override the withGraphFetched method on the model instead of manually retrieving/setting asset, wallet etc. everywhere. Would take some parsing of the original arguments or maybe some new arg to specify what to get off the cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think doing explicitly is better, overriding withGraphFetched might be a bit too hidden/"magic". Probably long term solution is just doing something like repository pattern and abstracting over ORM/db access, with an ability to add caching in front as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants