added batch logging by AceTheNinja · Pull Request #5 · bitfinexcom/bfx-facs-grc-slack

AceTheNinja · 2025-07-24T11:12:21Z

No description provided.

batrudinych · 2025-07-24T13:50:49Z

index.js

+      this._errorBatchTimer = null
+    }
+
+    this._processBatchedErrors().catch(err => {


please change linting here

batrudinych · 2025-07-24T13:58:30Z

README.md

+    "env": "development",
+    "errorBatching": { // optional
+      "interval": 60000,
+      "maxSize": 50,


is it possible to compose and send 50x4000 symbols to slack at once?

We can create and send a file in that case

batrudinych · 2025-07-24T14:00:30Z

index.js

+   * @param {Object} payload - Payload to log
+   * @param {...any} extra - Additional information to log
+   */
+  async batchLogErrorToSlack (reqChannel, err, functionName, payload, ...extra) {


Suggested change

async batchLogErrorToSlack (reqChannel, err, functionName, payload, ...extra) {

async logErrorEnqueue (reqChannel, err, ...extra) {

batrudinych · 2025-07-25T13:49:00Z

index.js

+      errorEntry.lastSeen = now
+      errorEntry.payloads.push(payload)
+
+      if (this._errorBatch.size >= this._errorBatchingConfig.maxSize) {


i don't think we should flush if buffer is full. We should evict oldest keys. Maybe it would be better to use lru here

batrudinych · 2025-07-25T20:01:55Z

index.js

+    message += `*Summary:* ${totalErrors} errors across ${errors.length} types (${timeRange})\n\n`
+
+    let truncated = false
+    for (const error of errors.slice(0, 10)) {


why do we slice?

If the message is too large the API will fail so have kept it to only 10 different error per function

what happens to the rest?

question still stands. I suggest we log only last to slack to fulfill length requirement

batrudinych · 2025-07-25T20:02:09Z

index.js

+    for (const error of errors.slice(0, 10)) {
+      if (truncated) break
+
+      message += `• *${error.errorMessage}* (${error.count}x)\n`


what about the rest message info? like extra, etc.

that goes in payload

batrudinych · 2025-08-08T12:15:09Z

index.js

    if (opts.conf) this.conf = opts.conf
+
+    this._errorBatchingConfig = {
+      interval: this.conf.errorBatching.interval || 60000,


let's assume errorBatching may be absent completely cause we're not going to update all existing running places

added null coalesce

batrudinych · 2025-08-08T12:16:24Z

index.js

+   * @param {Object} payload - Payload to log
+   * @param {...any} extra - Additional information to log
+   */
+  async logErrorEnqueue (reqChannel, err, functionName, payload, ...extra) {


let's make parameter functionName more generalized, i.e. errorOrigin or originName or errorSource or sourceName etc. You can come up with something better.

batrudinych · 2025-08-08T12:16:36Z

index.js

+   * @param {Error} err - Error to log
+   * @param {string} functionName - Name of the function where the error occurred
+   * @param {Object} payload - Payload to log
+   * @param {...any} extra - Additional information to log


is extra a required parameter?

no we're just putting all other params in 1 object. It can be undefined also

Suggested change

* @param {...any} extra - Additional information to log

* @param {...any} [extra] - Additional information to log

batrudinych · 2025-08-08T12:17:25Z

index.js

+      let errorEntry = this._errorBatch.get(errorKey)
+
+      if (!errorEntry) {
+        errorEntry = {


extra is not used, what is it for then?

batrudinych · 2025-09-22T16:16:39Z

index.js


    if (opts.conf) this.conf = opts.conf
+
+    this._errorBatchingConfig = {


why do we store it again if we already have it in this.conf?
additionally, if it's omitted (in readme it's commented as optional), you gonna get error trying to access this.conf.errorBatching
if there is no this.conf.errorBatching, this instance is not using batching, we should not setup it. I think additional timer is an overhead cause not all workers need batching

removed extra variable and not setting up batch logging interval if config does not exist

batrudinych · 2025-09-22T16:16:59Z

index.js

+      maxMessageLength: this.conf.errorBatching?.maxMessageLength || 4000
+    }
+
+    this._errorBatch = new LRU({


if there is no this.conf.errorBatching, we should not setup it

batrudinych · 2025-09-22T16:18:14Z

index.js

+      max: this._errorBatchingConfig.maxSize
+    })
+
+    this._initErrorBatching()


if there is no this.conf.errorBatching, we should not setup it

batrudinych · 2025-09-22T16:25:43Z

index.js

+   * @param {Error} err - Error to log
+   * @param {string} functionName - Name of the function where the error occurred
+   * @param {Object} payload - Payload to log
+   * @param {...any} extra - Additional information to log


Suggested change

* @param {...any} extra - Additional information to log

* @param {...any} [extra] - Additional information to log

batrudinych · 2025-09-22T22:03:13Z

test/facility.test.js

pls create folder test and put a file there named facility.test.js

batrudinych · 2025-09-22T22:03:38Z

package.json

+    "lru": "^3.1.0"
  },
  "devDependencies": {
+    "@types/jest": "^30.0.0",


you need it?

needed it for local syntax highlighting. Removed it

batrudinych · 2025-09-22T22:06:49Z

package.json

  "dependencies": {
-    "bfx-facs-base": "git+https://github.com/bitfinexcom/bfx-facs-base.git"
+    "bfx-facs-base": "git+https://github.com/bitfinexcom/bfx-facs-base.git",
+    "lru": "^3.1.0"


let's lock dep and add package-lock.json (use node 16.20)

batrudinych · 2025-09-22T22:07:42Z

index.js

+    const earliest = new Date(Math.min(...allTimes))
+    const latest = new Date(Math.max(...allTimes))
+
+    const formatTime = (date) => date.toISOString().substring(11, 19)


looks like a utility function that should not be re-defined inside a function each time

moved to utils

batrudinych · 2025-09-22T22:11:00Z

index.js

+        await this._sendBatchedErrorMessage(reqChannel, sourceName, errors)
+      }
+
+      this._errorBatch.clear()


if _sendBatchedErrorMessage fails, we never clear?

now it should, added clear to finally block

- batching init is now conf based - moved test to test/facility.test.js - clear batch if sending fails so error don't pile up

AceTheNinja · 2025-10-07T07:06:25Z

#5 (comment)

cannot have optional parameter with spread operator. If no args are present it will be empty

avsek477 · 2025-10-14T06:35:14Z

package.json

  "dependencies": {
-    "bfx-facs-base": "git+https://github.com/bitfinexcom/bfx-facs-base.git"
+    "bfx-facs-base": "git+https://github.com/bitfinexcom/bfx-facs-base.git",
+    "lru": "3.1.0"


why not use facs-lru ?

adding another fac that implements base and starting it in the constructor felt like an unnecessary complication for something simple like this

@AceTheNinja let's add an optional lru fac dependency for slack for the cases when we enable batch logging

vigan-abd · 2025-10-21T14:12:21Z

index.js

+    }
+
+    try {
+      await this._processBatchedErrors()


why this is called on stop? wouldn't it hang for a long time possibly?

i think it's called to flush the last batch of errors, otherwise it'll be lost
not sure about long time, i think it should be reasonable time since it's an api call. We can limit the call / stop function in time and exit abruptly if we exceed graceful exit limit.

vigan-abd · 2025-10-21T14:14:53Z

utils/date-time.js

use strict missing

pushed an update

vigan-abd · 2025-10-21T14:17:49Z

index.js


    if (opts.conf) this.conf = opts.conf
+
+    if (this.conf.errorBatching && opts.lru) {


why are we using lru and not map? with lru we would lose data right?

i think in order to keep the buffer fixed in length. If a worker starts to spam, we don't want to keep the spam rather than track let's say last 100 messages of a certain type.
for instance, a worker sends error messages for different occasions, i.e. API call failure and processing issue. Let's say it reports 1 api call failure and 1000 processing issues, We want both types to be reported, however we don't want 1000 processing issues (same type issue) being reported, we cut it down to, let's say, 10 only

that was the idea behind this update in general. If a worker starts spamming, this logic would still allow us to know about the problem, however, would cut down api calls and payload significantly.

anyways, if i missed the purppose, @AceTheNinja please clarify

couldn't have explained it better 🫡 @batrudinych

index.js

batrudinych · 2025-11-04T17:01:30Z

index.js

+  }
+
+  _createErrorKey (reqChannel, err, sourceName) {
+    const errorMsg = err?.message || 'Unknown error'


check this

maybe you compute hash of stringified error instead of using it directly?

batrudinych · 2025-11-04T17:06:06Z

index.js

+
+      errorEntry.count++
+      errorEntry.lastSeen = now
+      errorEntry.payloads.push({ payload, extras: extra })


you're adding same stuff twice if you just initialized cache entry.

btw if we spam under same errorKey, aren't we bloating the payloads array? Size should be restricted, otherwise we can run out of memory

batrudinych · 2025-11-04T17:06:29Z

index.js

+          payloads: [
+            { payload, extras: extra }
+          ],
+          count: 0,


is it 0? you just seen it first time

batrudinych · 2025-11-04T17:07:01Z

index.js

+    }
+  }
+
+  _createErrorKey (reqChannel, err, sourceName) {


make a default value for sourceName

batrudinych · 2025-11-04T17:24:42Z

index.js

+        message += payloadStr
+      }
+
+      if (error.payloads.length > 3) {


pls no magic numbers

batrudinych · 2025-11-04T17:24:49Z

index.js

+      }
+    }
+
+    if (errors.length > 10 && !truncated) {


no magic numbers

batrudinych · 2025-11-04T17:27:19Z

index.js

+  }
+
+  async _sendBatchedErrorMessage (reqChannel, sourceName, errors) {
+    const totalErrors = errors.reduce((sum, error) => sum + error.count, 0)


you can keep the counter separately to avoid computing it here, 1 less array pass

batrudinych · 2025-11-04T17:28:10Z

index.js

+            { payload, extras: extra }
+          ],
+          count: 0,
+          firstSeen: now,


looks like you use these dates just to know when you got first and last errors. Use separate properties, do not store inside the object and avoid excessive array passes

batrudinych · 2025-11-04T17:29:28Z

package.json

    "bfx-facs-base": "git+https://github.com/bitfinexcom/bfx-facs-base.git"
  },
+  "optionalDependencies": {
+    "bfx-facs-lru": "git+https://github.com/bitfinexcom/bfx-facs-lru.git"


do we need it here? it should come from outside as dependency when we instantiate the fac, i think

Co-authored-by: batrudinych <batrudinych@gmail.com>

…truncation logic

added batch logging

334f7e7

batrudinych reviewed Jul 25, 2025

View reviewed changes

added lru based implementation

f8e2e8b

batrudinych reviewed Aug 8, 2025

View reviewed changes

AceTheNinja added 3 commits August 14, 2025 12:49

functionName param change

9009666

added extra to logs

49daf8b

removed unnecessary comments

d8daa5d

batrudinych reviewed Sep 22, 2025

View reviewed changes

AceTheNinja added 3 commits October 7, 2025 11:01

- improved conf handling

610a023

- batching init is now conf based - moved test to test/facility.test.js - clear batch if sending fails so error don't pile up

added package-lock with node 16

2e2ba0e

removed @types/jest from devDependencies

01c62e2

avsek477 reviewed Oct 14, 2025

View reviewed changes

refactor: add lru and optional argument to fac

058e1cd

vigan-abd reviewed Oct 21, 2025

View reviewed changes

chore: enable strict mode in date-time utility

e4cc258

batrudinych reviewed Nov 4, 2025

View reviewed changes

AceTheNinja and others added 6 commits November 5, 2025 10:45

Update index.js

7c00406

Co-authored-by: batrudinych <batrudinych@gmail.com>

refactor: update error batching logic and key generation

3743ce5

Merge branch 'main' of github.com:AceTheNinja/bfx-facs-grc-slack

be446fa

removed array passes

f9df13b

refactor: added error batching config validation and improve message …

b195ad6

…truncation logic

removed optional deps

6186fc6

	async batchLogErrorToSlack (reqChannel, err, functionName, payload, ...extra) {
	async logErrorEnqueue (reqChannel, err, ...extra) {

	* @param {...any} extra - Additional information to log
	* @param {...any} [extra] - Additional information to log


		if (opts.conf) this.conf = opts.conf

		this._errorBatchingConfig = {


		if (opts.conf) this.conf = opts.conf

		if (this.conf.errorBatching && opts.lru) {

Conversation

AceTheNinja commented Jul 24, 2025

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!

AceTheNinja Jul 28, 2025 • 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!

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!

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!

AceTheNinja commented Oct 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AceTheNinja Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

AceTheNinja Jul 28, 2025 •

edited

Loading

AceTheNinja Oct 14, 2025 •

edited

Loading

AceTheNinja Oct 28, 2025 •

edited

Loading