axiomhq / axiom-js

Official language bindings and library extensions for Axiom
https://axiom.co
MIT License
85 stars 13 forks source link

fix: add safe stringify to format winston events #189

Closed lucassmuller closed 1 month ago

lucassmuller commented 1 month ago

Issue: Current JSON.stringify doesn't support bigints. This results in logs not being sent to Axiom.

Solution: Implement toJSON function in BigInt prototype so they can be stringified.

schehata commented 1 month ago

I updated the pnpm lock file on main, can you please rebase this PR, I will approve the CI again once its ready.

lucassmuller commented 1 month ago

I updated the pnpm lock file on main, can you please rebase this PR, I will approve the CI again once its ready.

done

btw the root cause of this problem is actually the following code line

https://github.com/axiomhq/axiom-js/blob/70c846719f3b4fd2e05475787f368af5189f476b/packages/js/src/client.ts#L163

probably i should adjust it there. thoughts?

schehata commented 1 month ago

I updated the pnpm lock file on main, can you please rebase this PR, I will approve the CI again once its ready.

done

btw the root cause of this problem is actually the following code line

https://github.com/axiomhq/axiom-js/blob/70c846719f3b4fd2e05475787f368af5189f476b/packages/js/src/client.ts#L163

probably i should adjust it there. thoughts?

yeah, here it would with other transports as well, not just winston. go for it 👍

Do you have a test case that we can apply here?

lucassmuller commented 1 month ago

I updated the pnpm lock file on main, can you please rebase this PR, I will approve the CI again once its ready.

done btw the root cause of this problem is actually the following code line https://github.com/axiomhq/axiom-js/blob/70c846719f3b4fd2e05475787f368af5189f476b/packages/js/src/client.ts#L163

probably i should adjust it there. thoughts?

yeah, here it would with other transports as well, not just winston. go for it 👍

Do you have a test case that we can apply here?

Done

schehata commented 1 month ago

thanks @lucassmuller , that looks great. It will be in the upcoming release.