feat(localenv): add performance metrics #2999

BlairCurrey · 2024-09-23T19:30:01Z

Changes proposed in this pull request

added timer to metric service
used new timer to power several new graphs tracking durations for most of the main paths in the create-outgoing-payment.js k6 script.

Partial view of local dashboard (best observed locally while running performance test):

Context

fixes #2983

currently set to merge into bc/2980/refactor-telemetry-optionality because it depends on those changes.

Checklist

Related issues linked using fixes #number
Tests added/updated
Make sure that all checks pass
Bruno collection updated (if necessary)
Documentation issue created with user-docs label (if necessary)
OpenAPI specs updated (if necessary)

- the underlying metrics for the new promql queries/grafana visualizations do not exist yet and should be added in later commit

- liquidity resolver, op lifecycle, op service create

…rformance-metrics

netlify · 2024-10-01T18:55:13Z

✅ Deploy Preview for brilliant-pasca-3e80ec ready!

Name	Link
🔨 Latest commit	`7f07761`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/672a47c18959590008365a60
😎 Deploy Preview	https://deploy-preview-2999--brilliant-pasca-3e80ec.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mkurapov

A few comments

mkurapov · 2024-10-07T11:12:53Z

packages/backend/src/open_payments/payment/outgoing/worker.ts

  try {
    await lifecycle.handleSending(deps, payment)
+    stopTimer()


could use a finally here

mkurapov · 2024-10-07T20:23:47Z

packages/backend/src/graphql/resolvers/liquidity.ts

    if (!event || !isOutgoingPaymentEvent(event)) {
+      stopTimer()


Something we don't necessary have to tackle in this PR, but what can we do when it comes to stopping timers during a happy path vs an error? is there something we can add to make that distinction in the metrics?

Yeah we could have a success: boolean attribute on the underlying histogram. I was inspired by a pattern that was shown to me during the WW, and it had that.

Not used currently, but it should be possible now. I updated the closure returned from startTimer to accept additionalAttributes which will be merged on the ones initially provided to startTimer. Usage example added to tests (and tests added for startTimer in general - noticed we didnt have any).

mkurapov · 2024-10-07T20:24:05Z

packages/backend/src/graphql/resolvers/liquidity.ts

    if (isFundingError(paymentOrErr)) {
+      stopTimer()


maybe a finally would be nice here to avoid calling stopTimer() multiple times

good idea. put everything in a new try and removed the stopTimers. catch just re-throws and finally calls stopTimer. This is a good pattern for this sort of thing... ensures we dont miss a place to stop the timer.

mkurapov · 2024-10-07T20:26:10Z

packages/backend/src/open_payments/payment/outgoing/worker.ts

@@ -79,10 +91,15 @@ async function handlePaymentLifecycle(
    return
  }

+  const stopTimer = deps.telemetry.startTimer('handle_sending_ms', {
+    callName: 'handleSending'


what do you think about prefixing callName with service name always?

messaged you on this with more details. seems like a good idea but if we want to make it a standard prefix I think maybe it needs to be more abstract than service name since not everything is called from outside the service method or is necessarily scoped to a service (gql resolvers,accountService name doesnt properly signify tb/psql, etc.)

updated to prefix with a pattern like [AbstractScopeName]:literalFunctionName.

for example:

WebhookService:getResourceById

Resolver:depositOutgoingPaymentLiquidity

Not every case is as clean as service calls where we have an obvious and unambiguous scope so I'm keeping it a little more abstract to ensure we can standardize.

packages/backend/src/open_payments/quote/service.ts

mkurapov · 2024-10-07T20:28:29Z

packages/backend/src/payment-method/ilp/service.ts

+      callName: 'rates',
+      description: 'Time to get rates'
+    }
+  )
  const rates = await deps.ratesService
    .rates(options.walletAddress.asset.code)


maybe best to just convert to a normal try-catch call?

converted from chaining to try/catch/finally

Co-authored-by: Max Kurapov <[email protected]>

BlairCurrey · 2024-10-16T15:15:42Z

packages/auth/src/graphql/schema.graphql

-    "Unique identifier of the grant."
-    id: ID!
-  ): Grant!
+  grant("Unique identifier of the grant." id: ID!): Grant!


some formatting issue made it into main somehow. had to format and generate gql types to make CI happy here.

If you merge in main this should go away

mkurapov

Minor comments

mkurapov · 2024-10-31T14:30:25Z

packages/auth/src/graphql/schema.graphql

-    "Unique identifier of the grant."
-    id: ID!
-  ): Grant!
+  grant("Unique identifier of the grant." id: ID!): Grant!


If you merge in main this should go away

mkurapov · 2024-10-31T14:31:16Z

packages/backend/src/payment-method/ilp/service.ts

+    {
+      callName: 'RateService:rates',
+      description: 'Time to get rates'
+    }


Should this be directly in the rates service instead? Or instead, called something like PaymentMethod:ILP:getQuote:getRates?

Hmm fair point.

We have external timers that wrap function calls from the outside like this, and internal timers that start at the beginning of a function and stop at the end. I think in this case we want the external timer because the goal is to measure getQuote and it's parts (thats how this metric is graphed). In other words, we want to measure this rates service call, not all of them.

I take your point on the name. I was thinking about it in the context of the graph, where the PaymentMethod:ILP:getQuote would be redundant. However, as soon as we want to time this same method somewhere else the name breaks down, so yeah... looks like that convention (scoped by where its called) is good.

In other words, we want to measure this rates service call, not all of them.

Yes, makes sense. We do have the histogram name ilp_get_quote_rate_time_ms, so that is easy to narrow down and trace in the code in any case.

For the callName, just as long as we can get group metrics from a certain part of the codebase like PaymentMethod:ILP:* that's good enough.

BlairCurrey added 10 commits September 19, 2024 13:35

refactor(backend): add NoopTelemetryServiceImpl to make tel optional

5de7ad9

refactor(backend): rm unecessary telemetry exists check

751ffa9

fix(localenv): rm telemetry log noise

a003c3e

feat(backend): add startTimer tel method

10b4ea4

feat: update local grafana dash w/ performance metrics

6c660ad

- the underlying metrics for the new promql queries/grafana visualizations do not exist yet and should be added in later commit

feat(backend): add telemetry service dep to services

46e00aa

feat(backend): add accounting service timer metrics

bbddc36

feat(backend): add service timer metrics

ff3fd70

- liquidity resolver, op lifecycle, op service create

feat(backend): add more performance timer metrics

6357f79

chore: rename performance metrics, update dashbaord

0b83806

BlairCurrey changed the title ~~Bc/2983/performance metrics~~ feat(localenv): add performance metrics Sep 23, 2024

github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels Sep 23, 2024

BlairCurrey added 5 commits September 23, 2024 15:31

chore: format

29143f4

fix(backend): rm problematic connector middleware timers

522b339

fix(backend): fix deposit outgoing payment liquidity performance metrics

3033c2a

chore: format

0ede073

chore: rename metric for case

a3efc73

BlairCurrey requested review from mkurapov and koekiebox September 24, 2024 17:30

BlairCurrey marked this pull request as ready for review September 24, 2024 17:31

fix(backend): balance tests, missing tel dep

84f825f

github-actions bot added the type: tests Testing related label Sep 24, 2024

BlairCurrey added 7 commits September 24, 2024 14:09

feat(backend): switch tel servive impl in dep definition

dfd8b2b

Merge branch 'main' into bc/2980/refactor-telemetry-optionality

c8753fc

Merge branch 'bc/2980/refactor-telemetry-optionality' into bc/2983/pe…

f70cc29

…rformance-metrics

fix(backend): test deps setup

b1b1866

chore(backend): cleanup test

87e73d3

chore: restore erroneously rm test, fix dep

e050a40

fix(localenv): dashboard visualization name

328ebbe

Base automatically changed from bc/2980/refactor-telemetry-optionality to main October 1, 2024 14:12

BlairCurrey added 2 commits October 1, 2024 14:51

chore: format

5c06039

Merge branch 'main' into bc/2983/performance-metrics

b5143b7

fix(backend): rm dupe class from merge

fb19aeb

mkurapov reviewed Oct 7, 2024

View reviewed changes

BlairCurrey and others added 6 commits October 15, 2024 11:35

Update packages/backend/src/open_payments/quote/service.ts

4ae9649

Co-authored-by: Max Kurapov <[email protected]>

refactor(backend): use try/catch/finally to enforce stopTimer

2ad883b

feat(backend): support additional attributes on stopTimer and add tests

24a5eff

chore: format

ba42a29

Merge branch 'main' into bc/2983/performance-metrics

6803522

chore: format

ef85d8b

github-actions bot added the pkg: auth Changes in the GNAP auth package. label Oct 15, 2024

BlairCurrey added 2 commits October 15, 2024 13:41

chore: generate auth gql

658f85f

refactor(backend): timer callNames to include scope

03d929b

BlairCurrey requested a review from mkurapov October 16, 2024 14:48

BlairCurrey commented Oct 16, 2024

View reviewed changes

fix(localenv): rm deprecated otel collector log config

ed1b5d3

mkurapov reviewed Oct 31, 2024

View reviewed changes

Merge branch 'main' into bc/2983/performance-metrics

7f07761

github-actions bot removed the pkg: auth Changes in the GNAP auth package. label Nov 5, 2024

mkurapov approved these changes Nov 6, 2024

View reviewed changes

BlairCurrey merged commit aa77aea into main Nov 7, 2024
30 of 42 checks passed

BlairCurrey deleted the bc/2983/performance-metrics branch November 7, 2024 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(localenv): add performance metrics #2999

feat(localenv): add performance metrics #2999

BlairCurrey commented Sep 23, 2024 •

edited

Loading

netlify bot commented Oct 1, 2024 •

edited

Loading

mkurapov left a comment

mkurapov Oct 7, 2024

mkurapov Oct 7, 2024

BlairCurrey Oct 15, 2024 •

edited

Loading

BlairCurrey Oct 15, 2024

mkurapov Oct 7, 2024

BlairCurrey Oct 15, 2024

mkurapov Oct 7, 2024

BlairCurrey Oct 15, 2024

BlairCurrey Oct 16, 2024

mkurapov Oct 7, 2024

BlairCurrey Oct 15, 2024

BlairCurrey Oct 16, 2024 •

edited

Loading

mkurapov Oct 31, 2024

mkurapov left a comment

mkurapov Oct 31, 2024

mkurapov Oct 31, 2024

BlairCurrey Nov 5, 2024

mkurapov Nov 5, 2024

feat(localenv): add performance metrics #2999

feat(localenv): add performance metrics #2999

Conversation

BlairCurrey commented Sep 23, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented Oct 1, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec ready!

mkurapov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented Sep 23, 2024 •

edited

Loading

netlify bot commented Oct 1, 2024 •

edited

Loading

BlairCurrey Oct 15, 2024 •

edited

Loading

BlairCurrey Oct 16, 2024 •

edited

Loading