Vendor intl NumberFormat

[denrase]

#skip_changelog

📜 Description

#skip-changelog

Closes #1241

💡 Motivation and Context

Implement specific method to format sample rate.

💚 How did you test it?

Unit tests with data created form NumberFormat output.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
• I updated the docs if needed
• All tests passing
• No breaking changes

🔮 Next steps

We could come up with more input/output string combinations.

[codecov]

Codecov Report

Patch coverage: 80.86% and project coverage change: +2.26 🎉

Comparison is base (8a7f528) 88.67% compared to head (510d90d) 90.94%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1269      +/-   ##
==========================================
+ Coverage   88.67%   90.94%   +2.26%     
==========================================
  Files         136      158      +22     
  Lines        4187     5058     +871     
==========================================
+ Hits         3713     4600     +887     
+ Misses        474      458      -16     

Impacted Files	Coverage Δ
dart/lib/src/utils/sample_rate_format.dart	80.70% <80.70%> (ø)
dart/lib/src/sentry_tracer.dart	95.30% <100.00%> (-0.04%)	⬇️

... and 59 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[denrase]

@marandaneto I randomly generated numbers to see if the formatting is the same. I have encountered an oddity from number formatter, where the 16th place after the decimal is rounded up/down. See below for a comparison. To me it looks that NumberFormat is doing the wring thing, as it is changing the input where it's not necessary. Sometimes it's rounding values >= 5 up, and sometimes down, which might come down to double imprecision in the algorithms.

Input	NumberFormat	SampleRateFormat
0.9805006842689749	0.9805006842689748	0.9805006842689749
0.9778557462002015	0.9778557462002016	0.9778557462002015
0.9752429342800975	0.9752429342800974	0.9752429342800975
0.9590017725063013	0.9590017725063012	0.9590017725063013

[denrase]

Opted to vendor all the needed files with just minimal changes in the imports and folder structure. It's a lot of code just to get the exact same result as before on the 16th place after the comma. I'm not sure if this is worth it TBH.

[marandaneto]

Opted to vendor all the needed files with just minimal changes in the imports and folder structure. It's a lot of code just to get the exact same result as before on the 16th place after the comma. I'm not sure if this is worth it TBH.

I get the trade off and is indeed a lot of code, but if using the previous implementation, we need to be sure that we are not running into rounding issues, the output should be the same as the intl parse, can we do that?

[denrase]

@marandaneto Only thing we could try is to generate a larger number of random sample values in the tests, including intl in the test target and compare the outputs, using a range that we ate comfortable with as rounding error. 🤔

[marandaneto]

@marandaneto Only thing we could try is to generate a larger number of random sample values in the tests, including intl in the test target and compare the outputs, using a range that we ate comfortable with as rounding error. 🤔

mmm, the question is if we can avoid the routing error, unless intl does have the very same problem?

[marandaneto]

@chadwhitacre I'd like to double check if this is fine, License https://github.com/dart-lang/intl/blob/master/LICENSE BSD 3
Should we copy the license text on top of this file?

[chadwhitacre]

Yes, we actually just clarified the policy around this🔒 (in getsentry/team-ospo#120):

1. Separate file
2. Link to where we got it (attribution)
3. Include license text in file header

[marandaneto]

@denrase lets do this then, make CI happy and merge it.

[marandaneto]

Lets wait for https://github.com/getsentry/sentry-dart/pull/1269/files#r1142973732

[marandaneto]

@denrase

Expected: '0.1919191919191919'
Actual: '0.191919191919192'
Which: is different.
Expected: ... 191919191919
Actual: ... 19191919192
^
Differ at offset 16

@denrase is this fixed? I saw some commits but not sure.

[denrase]

@marandaneto I have some weird behaviour. Since the code is from intl, i included it again (in tests only) to see if it also behaves differently on ubuntu and windows. CI was green for all tests then. So either the test with the rounding error is flaky or something else is going on.

[denrase]

@marandaneto I was mistaken, the test with intl failed with the same result. So the behaviour is the same as before and also with the vendored code. There are slightly different outcomes on different platforms. The result below is from using intl:

[marandaneto]

@marandaneto I was mistaken, the test with intl failed with the same result. So the behaviour is the same as before and also with the vendored code. There are slightly different outcomes on different platforms. The result below is from using intl:

That's fine then, its similar to how it was before, let's compare the output of the intl with our code result instead of hardcoded values, this should fix the test because they should have the same result.

[denrase]

@marandaneto I'm not sure why this would fail here. AFAIK the code changes are not related to this test.

[marandaneto]

@marandaneto I'm not sure why this would fail here. AFAIK the code changes are not related to this test.

I already merged main into this branch, this is due to a new dio version, I already merged a fix for that.