-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): serialize bigint in metadata to string #619
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. I have a few suggestions.
child.addMetadata('key', BigInt(9007199254740991)); | ||
child.flush(); | ||
emitStub.should.have.been.calledOnce; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a similar test for Segment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing test harness for Segment stops at mocking the Segment.flush()
method and asserting that it's been called. Given that segment is serialized within the flush
method, the serialization is never done.
Adding this test would mean rearchitecting the tests and requires knowledge of the codebase that I don't have.
If the maintainers want to add this, I'd suggest them to either: 1/ address this in a future PR, 2/ contribute to this PR and provide the test fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this change! Left a few small comments, but otherwise looks good
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #619 +/- ##
==========================================
+ Coverage 83.41% 83.49% +0.07%
==========================================
Files 37 37
Lines 1797 1805 +8
==========================================
+ Hits 1499 1507 +8
Misses 298 298 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Carol Abadeer <[email protected]>
Co-authored-by: Carol Abadeer <[email protected]>
Hi @carolabadeer thanks for the review and the comments, I have committed both your suggestions! |
Issue #, if available: closes #616
Description of changes:
This PR introduces a new method called
serialize
to the prototype of theSegment
andSubsegment
objects. This method wraps theJSON.stringify()
call that was previously made by thetoString()
andformat()
methods of these two classes.When calling the
JSON.stringify()
function, the method also passes a replacer function. This replacer for now includes anif
statement that castsBigInt
objects tostring
. This avoids runtimes errors described in the linked issue.By extracting the method, customers can easily override it and extend the replacer to suit their needs.
In addition to the method, the PR includes also changes to the type definition and the addition of a test.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.