Fix logtail/logtail-js#19 Maximum call stack size exceeded

[boly38]

Fix #19 Maximum call stack size exceeded

Closes: 19
Thanks: Yago Tomé yagotome@gmail.com

---

this PR is my first typescript contribution so be indulgent. I take all comments. 🙏

[adikus]

Thanks a lot @boly38! This looks great and makes sense.

I left some comments but most of them are fairly small nitpicks.

I'll also try to check this out and run the test. (As you've noticed we don't have CI setup just yet)

[adikus]

Test looks great ✅

[boly38]

contributing on this package: my small return of experience: in option.

• first: in order to augment ILogtailOptions in core package, I do need to build type, then core package but I first encountered a lots of typescript issue when running npm run build in core and type packages. I understand that I need to increase typescript version, an regenerate package-lock via npm install to fix that.. I don't understand why I had to do that update if the build is green ?

• while working on "snapshot" version on type, then core to update node package, I need a tip. This was not easy to proceed (at least for me). Here is a minimal tips (I omit npm install everywhere..)

cd packages/types && npm install && npm run build
cd ../core && npm install && npm link ../types && npm run build
cd ../node && npm link ../core && npm link ../types && npm run build
cd ../../ && npm run test

there is maybe a shortest way to contribute.
Is it possible to produce a minimal contribute.md with howTo + requirements to build (at least) type + core + node package ; like for example this 3 previous line as example ? the goal is to guide TS noob like me :p

• in bonus, I would like to run test (jest) for only some packages and not all all time ; this without aving to update jest filter (jest.config.js). I don't know if there is a simple way to do that. Could be added to how to contribute tips.

[adikus]

@boly38 Thanks for the feedback!

We have Lerna to manage the multiple packages, however last time it I tried to get it working, I wasn't able to.

From what I understand Lerna is supposed to create symlinks in each package's node_modules to the other Logtail packages it needs - however this functionality was broken for me (something in our config might not be just right). I was able to create those symlinks manually though and get individual tests working that way (I think it was something like ln -s ../../packages/ node_modules/@logtail e.g. from inside packages/node; EDIT: Although using the npm link command as you did is much cleaner 😅 ).

To run individual jest tests, for checking the test you wrote I just run ./node_modules/.bin/jest packages/node/src/node.test.ts.

Hope that helps you get going on this PR, but I would completely understand if you don't want to bother - in that case I'm happy to take this PR over 🙂

We definitely should spend some more time to get the Lerna setup working, because as I noted, the current setup is not great.

Thanks!

[boly38]

Thanks for your tips : I just fixed base test (I think).

I will squash those 3 commit and re-push, then give you the hand on this PR.
I'm not attached to the ownership of this contribution, so feel free to amend, refactor as you wish.
What is important for me is that the fix arrives in master :)
have a good day

[yagotome]

Hi there! I'm the OP of this issue, thanks a lot for following up on it.

I'm keen to share some thoughts with you guys here. I appreciate there are other reasons you need to control max depth in the context object, as the underlying serialization scheme (msgpack) also has its own limitation, as pointed out by @boly38 in this comment. However, I wanted to call out that the original problem (#19) could be handled differently, in a more accurate way. That's how I believe the problem should be tackled:

1. Keep track of all object references (including array) seen at the function call stack -- a WeakSet could be used for that.
2. If you visit the same object again, then you refuse to serialize it.

Just a draft snippet to make it clearer:

private sanitizeForEncoding(value: any): any {
    const dfsPath = new WeakSet(); // note the weakset to track a dfs path (objects seen in the current call stack)

    if (value === null || typeof value === "boolean" || typeof value === "number" || typeof value === "string") {
      return value;
    } else if (value instanceof Date) {
      return value.toISOString();
    } else if (Array.isArray(value)) {
      return value.map((item) => this.sanitizeForEncoding(item));
    } else if (typeof value === "object") {
      const logClone: { [key: string]: any } = {};

      Object.entries(value).forEach((item: object) => {
        const key = item[0];
        const value = item[1];

        if (dfsPath.has(value)) { // here we detected a cycle
          logClone[key] = value.toString();// result in "[object Object]" : we refused to serialize deeper
        } else {
          const result = this.sanitizeForEncoding(value);
          if (result !== undefined){
            logClone[key] = result;
          }
        }
      });

      dfsPath.delete(value); // the  value` object is leaving the call stack and will not be part of a cycle if saw again in a different dfs path

      return logClone;
    } else {
      return undefined;
    }
  }

That way, circular references would be stopped as soon as detected, instead of serializing them until they reach maximum call depth. What do you guys think? cc @adikus

[boly38]

my quick feedback :

• ℹ️ need to append dfsPath as arg for recursive call but understand the idea of weakSet & recursion ( with doc example in addition )
• ➕ elegent design, and fix the circular ref
• ➖ don't fix an object without circular ref but really deep (can be unrealistic I agree)

[adikus]

Thanks, I like the idea! I'll try to incorporate this in.

[adikus]

@boly38 Thanks! I'm currently working on #32, after I'm done there, I'll rebase this on there and finish it off. I'll also incorporate @yagotome's suggestion.

[adikus]

Moving this to #34