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
ref(tracing): Simplify sample_rate serialization for DSC #5475
Conversation
size-limit report 📦
|
packages/tracing/src/transaction.ts
Outdated
rate !== undefined | ||
? rate.toLocaleString('fullwide', { useGrouping: false, maximumFractionDigits: 16 }) | ||
: undefined; | ||
const sample_rate = rate !== undefined ? JSON.stringify(rate) : undefined; |
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.
Do we even need the extra JSON.stringify
call? Won't just doing sample_rate: rate
below be enough?
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.
Good point. We can simplify it further to that but I think we still have to convert it toString (if I understand this test correctly).
2775a4d
to
dd66615
Compare
This PR changes our formatting logic of the
sample_rate
value when the dynamic sampling context (DSC) is populated. Instead of usingNumber.toLocaleString
with a bunch of options (which theoretically is supported by all browsers; but it turns out that IE11 doesn't support thefullwide
locale we use), we now simply convert the numbertoString
to do the job. Given that Relay now supports parsing the sample_rate from JSON format, we can drop our previous formatting logic.Deleted a test that's IMO no longer necessary, covering the conversion of exponential to decimal notation.
Fixes #5466