Implement throttler for non-core db functions to improve postgres performance - #318

JonoPrest · 2024-11-04T12:46:54Z

Adds debounce module to the envio package
Moves Pino bindings and Utils to envio package
Adds rescript-schema to envio package
Uses debounce instead of LazyWriter for Benchmark.res
Uses debounce instead of async task queue for update chain metadata
Applies debounce to cleanup db functions
Adds tests for debounce module

DZakh

I didn't take at the Debouncer code yet. Just a few early comments, I'll continue in 30 minutes

DZakh · 2024-11-05T10:27:32Z

  "dependencies": {
-    "rescript": "11.1.3"
+    "rescript": "11.1.3",
+    "rescript-schema": "8.2.0"


We should keep the package.json.tmpl dependencies in sync. Need to add rescript-schema there as well

Ah good spot 👌🏼

DZakh · 2024-11-05T10:29:45Z

+let t = (logger, exn, message) =>
+  logger->Pino.errorExn(message->Pino.createPinoMessageWithError(exn))
+
+%%private(


Let's have a .resi file instead

Can you elaborate on why you think so?

IMO this is better for the given case unless we want opaque types (I'd prefer visible types to make it easier to test). This way you don't need to maintain 2 files and keep signatures/types matching.

Ok, I thought we want it to be opaque

I like opaque types from an API perspective, but it makes it less testable and more encapsulated. I'm keen to try not use too much encapsulation unless it solves footguns. At which point you can just have an inner module of exposed getter functions for testing.

I prefer the black-box approach for unit testing, when we test only the public API the way how the user would interact with it. This is more reliable since we test exactly how the code is used. Also, when some internal implementation changes, it doesn't require us to update the tests.

Fair, but it's still more boilerplate just to hide simple data etc. I'm wondering how much the black-box ceremony helps us vs just creates overhead.

I got used to writing the code this way, so I think this is not a big difference. But might be annoying sometimes

DZakh · 2024-11-05T10:41:28Z

+let t = (logger, exn, message) =>
+  logger->Pino.errorExn(message->Pino.createPinoMessageWithError(exn))


Is this used?

Nope, that was accidentally left in 👍🏼

DZakh

Looks good. But this is more like a throttler than a debouncer. Debounce restarts the timer every time a new function is scheduled, while the throttle keeps the original timer

DZakh · 2024-11-05T11:51:00Z

+    Assert.deepEqual(
+      actionsCalled,
+      [1, 2],
+      ~message="Scheduler should have been called straight after the initial interval",


This is Throttler then, not a Debouncer

Kind of but if you look at the first test it also has debouncing properties. Open to renaming but this is the behaviour I was looking for.

Maybe Debottler? 🙃

Jk, I think we can call it Throttler, it makes more sense 👍🏼

DZakh · 2024-11-05T11:57:27Z

+      if timeSinceLastRun >= debouncer.intervalMillis {
+        debouncer.isRunning = true
+        debouncer.scheduled = None
+        debouncer.lastRunTimeMillis = Js.Date.now()


Should we maybe set it after the fn call?

The way I see it, it's not a cooldown period so much as an interval, it can work either way it just makes sense in my head to have a time interval from when the last function was called, not necessarily finished.

Say a cleanup function gets called on the postgres and takes really long awaiting a transaction. By the time it's finished we can immediately execute the next scheduled function if it's passed the interval.

DZakh · 2024-11-05T12:00:08Z

+        await Js.Promise2.make((~resolve, ~reject as _) => {
+          let _ = Js.Global.setTimeout(() => {
+            debouncer.isAwaitingInterval = false
+            resolve()
+          }, Belt.Int.fromFloat(timeOutInterval))
+        })
+
+        await debouncer->startInternal


I'd say we don't need a promise here. You can call debouncer->startInternal directly from Js.Global.setTimeout

True! It's just then that the resolve of startInternal is not indicative of it actually finishing.

DZakh · 2024-11-05T12:01:44Z

+      let logger = Logging.createChild(
+        ~params={
+          "context": "Debouncer for chain metadata writes",
+          "intervalMillis": intervalMillis,
+        },
+      )
+      Debouncer.make(~intervalMillis, ~logger)


I think this is convenient here, but isn't it more correct to pass the logger from where we call schedule, instead of when we create the Debouncer

It can work either way, then we just store the logger here in this state and pass where it get's called. I guess it's just a preference of where we store the logger. I think it's better than constructing a new logger on every call unless we want to pass in different context values, which might be nice as well 🤔

Actually after looking back at the code, it is best this way.

startInternal will recurse through scheduled calls, so if that takes the logger as a param then it can only hand down the same logger even if it's a different scheduled call.

If you actually want a logger to be tied to where it gets scheduled then the "scheduled" state should also have a logger. I think we can do that but it's not needed yet.

DZakh

👍

JonoPrest added 10 commits November 4, 2024 12:38

Add debouncer module with test

8e8225c

Move Pino bindings and Utils to envio package

a6b1276

Use Pino logger in debouncer

c1ce1f3

Update test to use logger

8825cbf

Update name from delayMillis to intervalMillis

3e52a15

Use debouncer instead of lazy writer for Benchmark module

e161615

Move chainMetaData updates to using a debouncer

d83e6d1

Add debouncing for db cleanup functions

5cae77f

Fix debouncer and add more tests

ff11ba3

Improve debouncer and add test

3146305

JonoPrest requested a review from DZakh November 4, 2024 12:46

DZakh requested changes Nov 5, 2024

View reviewed changes

JonoPrest added 2 commits November 5, 2024 13:23

Remove unused Debouncer function

fffc690

Add rescript schema to package.json.tmpl

f35c7dc

DZakh reviewed Nov 5, 2024

View reviewed changes

Rename debouncer to throttler everywhere

8c4507a

JonoPrest requested a review from DZakh November 5, 2024 16:20

Add resi file with function docs

401dfa6

JonoPrest force-pushed the jp/implement-debounce branch from 96282e3 to 401dfa6 Compare November 5, 2024 16:58

Remove promise from timeout

1bf51a7

DZakh approved these changes Nov 6, 2024

View reviewed changes

JonoPrest merged commit b4b2894 into main Nov 6, 2024

JonoPrest deleted the jp/implement-debounce branch November 6, 2024 08:04

JonoPrest changed the title ~~Implement Debounce for Cleanup & UI db functions~~ Implement throttler for non-core db functions to improve postgres performance Nov 6, 2024

		let t = (logger, exn, message) =>
		logger->Pino.errorExn(message->Pino.createPinoMessageWithError(exn))

Uh oh!

Conversation

JonoPrest commented Nov 4, 2024

Uh oh!

DZakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DZakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DZakh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JonoPrest Nov 5, 2024 •

edited

Loading