DF-24268 Cover bigint conversion in log censoring - #883
Conversation
|
👋 alejoberardino, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
NPM Publishing labels 🏷️🟢 This PR has valid version labels and will cause a |
| // Retry with a bigint-safe replacer in case the failure was due to a bigint value. | ||
| // JSON.stringify with a replacer function is slower, so we only pay that cost when | ||
| // the fast path above actually fails, rather than on every call. | ||
| stringified = JSON.stringify(obj, (_key, value) => |
There was a problem hiding this comment.
Why don't we just do this in the 1st pass?
There was a problem hiding this comment.
To avoid the performance hit unless we run into errors, since this branch is not very common doesn't make sense to slow down by default
There was a problem hiding this comment.
Around twice as slow with the replacer, from a very quick poc benchmark I just ran. The call itself is fast for a small object (.1 us) but as they grow and the number of calls grow this can start becoming significant, especially on slow cpus
|
🚀 Successfully created version bump PR: #879 |
2 participants
Some EAs currently see [Unknown] issues from trying to log objects with bigints, and JSON serialization fails. Since logs pass through the censoring method here, we can apply handling for this case in particular in there. We default to trying to serialize normally first, since that path will drop to the native JSON serializer impl which is much faster than the JS level one.