Option to flatten ingestion data #59
Conversation
|
The build step is failing, but it doesn't seem related to your changes, I reran the build step again, lets see how it goes. |
|
I checked the other PRs, build step is fine, can you double check the build error? |
|
I don't have a v18 node installed, it seems like on 19.x it passes and I use 20.x and it passes too. I'll get v18 at some point and try to run the tests again. |
|
I tried running it with 18.17 Node and all tests are passing just fine too. Not sure where to look for the issue. I'll look at the CI/CD Pipelines later, but it's not my strength. Below is the output of Node 18.17 passing pnpm test PS C:\Users\prokopec.simon\source\repos\prokopec-simon\axiom-js> node -v
╭────────────────────────────────────────────────────────────────────────╮ • Packages in scope: @axiomhq/js, @axiomhq/pino, @axiomhq/winston, examples-js, examples-pino, examples-winston, integration-tests, nextjs-app Tasks: 9 successful, 9 total |
|
I've tried to run the tests locally, one of them fails, something about fetch-retry. the main branch builds fine. not sure whats causing this yet. here is the error: |
|
oh, just noticed, the AxiomWithoutBatching class has the |
|
I think the issue has to do with the way you return the falttened data, and the syntax of async expect. and in the expect you are not actually returning the ingest method, and no awaiting for it: probably it would be |
|
Even though the changes should be minimal, I currently don't have the capacity to fix these problems. I'm using a fork that seems to work well, so it's probably just the tests. Once i have more free time I'll try to fix the issues. Thanks for your assistance. |
| @@ -45,6 +45,7 @@ | |||
| "dependencies": { | |||
| "cross-fetch": "^3.1.5", | |||
| "fetch-retry": "^5.0.3", | |||
| "flatted": "^3.2.7", | |||
There was a problem hiding this comment.
This needs to be bumped 3.2.9 is out. 🙏
| @@ -194,4 +194,17 @@ describe('Axiom', () => { | |||
| expect(response).not.toEqual('undefined'); | |||
| expect(response.matches).toHaveLength(2); | |||
| }); | |||
| it('should successfully handle circular JSON during ingestion with correct options', async () => { | |||
| const circularReferenceObject: any = { foo: 'bar', circularAttribute: null }; | |||
There was a problem hiding this comment.
Instead of disabling type checking by using any what if we use this? 🤔
type CircularReferenceObject = { foo: string; circularAttribute: CircularReferenceObject | null };
const circularReferenceObject: CircularReferenceObject = { foo: 'bar', circularAttribute: null };
circularReferenceObject.circularAttribute = circularReferenceObject;
| @@ -1,4 +1,4 @@ | |||
| lockfileVersion: '6.1' | |||
| lockfileVersion: '6.0' | |||
There was a problem hiding this comment.
The lock file shouldn't be downgraded, please update your pnpm install.
|
@prokopec-simon closing this as there has been no activity for a long time, feel free to reopen when ready. |
Option fixing a minor issue I came across and reported as #46
The main goal is allowing users to flatten data for logging if needed, mostly for preventing circular references in objects resulting in a Type Error when trying to JSON.stringify such data. Examples of naturally circular structures are Node's request object or some exception objects.
I've added it as an Ingestion option rather than resolving to it by default. The main reasons for that are that in many cases I would prefer to get an exception when trying to log circular data and figuring out why that's happening, the second one possibly being performance. I couldn't find any serious enough resources comparing the performance of default JSON.stringify and flatted stringify, although I reckon there shouldn't be much difference.
I decided to use a package for the flattening rather than trying to achieve it myself, even though it seems doable in just a few lines of code. I reckon that the added dependency and the tiny increase in size (0.5K) are worth it.