LdValue.Of( long ) is lossy

[JeffAshton]

Describe the bug LdValue stores numeric values as double, however the API supports long. long.MaxValue cannot be stored as a double.

To reproduce Best reproduced by a test:

        [Fact]
        public void CanUseLongTypeMaxValue()
        {
            long n = long.MaxValue;
            Assert.Equal(n, LdValue.Of(n).AsLong);
            Assert.Equal(n, LdValue.Convert.Long.ToType(LdValue.Of(n)));
            Assert.Equal(n, LdValue.Convert.Long.FromType(n).AsLong);
        }

Result:

Xunit.Sdk.EqualException
Assert.Equal() Failure
Expected: 9223372036854775807
Actual:   -9223372036854775808
   at LaunchDarkly.Sdk.LdValueTest.CanUseLongTypeMaxValue() in C:\D2L\opensource\launchdarkly\dotnet-sdk-common\test\LaunchDarkly.CommonSdk.Tests\LdValueTest.cs:line 479

Expected behavior Given that LdValue.Of( long ) exists, I would expect it to not be lossy. Newtonsoft.Json stored the actual value, and used decimal for comparisons of different types as a work around.

[eli-darkly]

@JeffAshton I understand your point, but the issue here isn't exactly the LdValue type. LaunchDarkly in general is not going to be able to handle numbers whose value is long.MaxValue, because those numbers get passed around as JSON and the native numeric type used when parsing JSON on nearly every platform is double-precision floating point. Having just the .NET SDK use a decimal type to store arbitrarily large numbers with no loss of precision would not really help.

[eli-darkly]

In other words, addressing this would have to be a cross-platform effort across every part of the platform— all of the SDKs, the LaunchDarkly application, the service endpoints. Going back to using Newtonsoft.Json in the .NET SDK would not accomplish anything.

[eli-darkly]

If you need to represent arbitrarily large numeric values in a feature flag or in user attributes, currently your only option is to use strings.

[eli-darkly]

I believe there's a page in process of being written for the main docs site that talks about how LaunchDarkly handles various JSON data types, including the numeric precision limitation. Once that's live, it would make sense for the API docs on LdValue to link to that.

[JeffAshton]

I think it should either just be supported or there should be some boundary checking for long. Personally I'd rather an overflow exception over the current behaviour.

[eli-darkly]

There's literally no way it can "just be supported" with the change you're suggesting; the product will not work as expected if you try to do that, totally aside from the specific implementation of the .NET SDK. A numeric value of that magnitude and precision cannot be represented in LaunchDarkly.

In a future version of the SDK we might start throwing exceptions for this kind of thing, but we have to be very careful. Flag evaluation and user-building is done so heavily and in so many code paths in LD-enabled applications that we have generally avoided the use of exceptions, as (especially in a language like C# where there is no equivalent to Java's checked exceptions, and therefore no way for the compiler to warn you that you're not accounting for a declared one) they would vastly increase the number of places where the application might be unexpectedly thrown out of its code path. I'd rather avoid getting into a philosophical discussion about that here because, again, it's not at all specific to this repo or this SDK but rather a product-wide design decision. But for instance, if you call one of the Variation methods and the flag doesn't exist, or is of the wrong data type, or various other problems, it doesn't throw an exception and I don't think we're likely to ever make it start doing so; developers have come to expect that these APIs never do. Basically the overall principle is that if the application is trying to do something the product doesn't support, it's still better to get a potentially somewhat wrong flag value than to prevent the application code from proceeding (except in very specific contexts like when you are initially configuring the SDK, since that's not something that's going to be happening in hundreds of places in your code). That depends of course on the developer being able to know what the product does or doesn't support, which depends on us documenting this kind of thing better.

[JeffAshton]

This might better illustrate the dangerous of mixing and matching floating point types:

LdValue floatValue = LdValue.Of( 0.3f );
LdValue doubleValue = LdValue.Of( 0.3 );
Assert.Equal( floatValue , doubleValue );

Result:

Xunit.Sdk.EqualException
Assert.Equal() Failure
Expected: 0.30000001192092896
Actual:   0.29999999999999999

This was handled correctly by Newtonsoft.Json.

[eli-darkly]

Double vs. float is a somewhat different issue than the one you originally raised, but the basic point is the same: switching to Newtonsoft.Json in this library would not change the behavior of every other part of the LaunchDarkly system, in which numbers are stored as double-precision floating point. I understand your concerns but it's simply not possible to address them with a change in this one SDK.

In the product as it currently exists, trying to represent numbers with precision beyond double will not work, period. If you need to store such numbers they must be strings. In fact, looking again at the API docs, I see that this exact issue is explicitly called out in the documentation for LdValue.of(long): "Note that the LaunchDarkly service, and most of the SDKs, represent numeric values internally in 64-bit floating-point, which has slightly less precision than a signed 64-bit long; therefore, the full range of long values cannot be accurately represented. If you need to set a user attribute to a numeric value with more significant digits than will fit in a System.Double, it is best to encode it as a string." Given that this behavior is clearly documented for this SDK, I think it's a bit unfair to say that you could not have expected it.

Similarly, in the product as it currently exists, it's inadvisable to expect accuracy for numbers that cannot be expressed accurately in any binary floating-point type, such as 0.3. The fact that you get non-matching values for 0.3 in float vs. double is well known, because there is not really such a value in either type, there are just very close values that will round to 0.3 depending on the output format. The only way to represent the value accurately is with a decimal type. And if the purpose of this SDK were to accurately capture any value that an application might want to put into JSON, then a decimal type would be appropriate. But the purpose of this SDK isn't to do that, nor to imitate Newtonsoft.Json's capabilities; the purpose is to facilitate the use of LaunchDarkly, a product in which numbers are stored in double-precision binary floating point. A number like 0.3f, even if the .NET SDK stored it exactly as you passed it, would be subject to rounding errors at many other points in the product outside of the SDK.

[eli-darkly]

I mean, there is still always room for improvement in documentation, and as I said we are in the process of highlighting this and similar issues on a cross-platform level in a page on docs.launchdarkly.com. And if there is some other place in the docs for this library where you think this issue could be called out more clearly, I welcome suggestions. But it certainly is called out already in the context of the specific method that you filed this issue for.

[eli-darkly]

Here's more specifically what I mean about "rounding errors at many other points in the product". There are three places where an arbitrary numeric value might appear: a flag variation, a value used with a rule operator (such as ">= 0.3"), or a user custom attribute. And in the case of a user custom attribute, there are two things it might be used for: comparing it to a value in a rule clause, and capturing it in analytics events. What would happen in each case if you tried to use a value like 0.3 or long.MaxValue is:

• For a flag variation, or a value in a rule clause, the number would lose precision immediately when you tried to set that value on the LD dashboard— either in the front-end JS code, or in the back-end Go code that would parse that number from a REST request body and represent it in Go's float64 type.
• For a user custom attribute, assuming the SDK was able to store the number with full precision, it would not work correctly with rule operators because the value it was compared to in the rule would have already lost precision. Then, when the attribute was sent in analytics data, the value would lose precision and be stored incorrectly because the analytics event service parses JSON data with the same Go behavior described above.

[eli-darkly]

So I hope you can see why I'm pushing back on this so strongly in terms of "Newtonsoft.Json could do this" not being the relevant point. Really even if the whole LaunchDarkly back end were rewritten to use decimal types for numbers, it wouldn't exactly help with this as long as those numbers are transmitted as JSON numbers (rather than changing the representations to instead send a string with some kind of tag indicating it's to be treated as a number), because we have many SDKs for platforms in which there is no such thing as a decimal type and no way to customize how JSON parsing is done. It's a known limitation of JSON as a data exchange format, so applications that use JSON but require this kind of precision generally avoid using the numeric type, because they can't be sure that whoever else is reading or writing the data can handle decimals as expected. In fact JavaScript, the language JSON was closely based on and was first implemented in, is barely any better in that regard (it does have a BigInt type but doesn't use it when parsing JSON).

[JeffAshton]

I can appreciate where you're coming from. I'm attempting to upgrade to the latest package and noticing this as a breaking change. The old implementation just benefitted from Newtonsoft.Json's handling of numeric types, and possibly better than all other Launch Darkly supported clients. I have a set of integration tests that caught it.

[eli-darkly]

I just don't understand how it can be a breaking change when the product doesn't support it. As far as I can see, using a value of long.MaxValue could not have ever worked in the context of actually using the product; the only thing that would've worked correctly, in the previous Newtonsoft-based implementation, was if you put that value into a user attribute and then converted the user to JSON but did not actually ever do anything LaunchDarkly-related with it.

[JeffAshton]

I just don't understand how it can be a breaking change when the product doesn't support it. As far as I can see, using a value of long.MaxValue could not have ever worked in the context of actually using the product; the only thing that would've worked correctly, in the previous Newtonsoft-based implementation, was if you put that value into a user attribute and then converted the user to JSON but did not actually ever do anything LaunchDarkly-related with it.

That's correct. Using the file source I was able to write boundary tests for long.MaxValue. In practice we don't have any values that come close. The only broken case that might existing as it stands today then is:

• Define a rule based on a numeric threshold (will get parsed as a double)
• Define a user with a custom attribute using LdValue.Of( float )

Edge case where the float to double conversion will cause problems. A work around is to avoid defining custom attributes as float, and always use double.

using System;
using System.Collections.Generic;
using System.IO;
using System.Text.Json;
using System.Threading;
using LaunchDarkly.Sdk;
using LaunchDarkly.Sdk.Server;
using LaunchDarkly.Sdk.Server.Integrations;

namespace LaunchDarklyFloatingPoint {

    class Program {

        static void Main() {

            const string path = "datasource.json";
            const string attributeName = "threshold";

            object flags = new {
                flags = new Dictionary<string, object>{
                    {
                        "test_key",
                        new {
                            key = "test_key",
                            deleted = false,
                            on = true,
                            offVariation = 1,
                            fallthrough = new {
                                variation = 1
                            },
                            rules = new [] {
                                new {
                                    variation = 0,
                                    clauses = new [] {
                                        new {
                                            attribute = attributeName,
                                            op = "lessThanOrEqual",
                                            values = new [] {
                                                0.3
                                            }
                                        }
                                    }
                                }
                            },
                            variations = new [] {
                                true,
                                false
                            },
                            version = 1
                        }
                    }
                }
            };

            string json = JsonSerializer.Serialize( flags, new JsonSerializerOptions {
                WriteIndented = true
            } );

            File.WriteAllText( path, json );

            FileDataSourceBuilder dataSource = new FileDataSourceBuilder()
                .AutoUpdate( false )
                .FilePaths( path );

            Configuration config = Configuration
                .Builder( sdkKey: "test" )
                .DataSource( dataSource )
                .Build();

            LdClient client = new LdClient( config );

            for(; ; ) {
                if( client.Initialized ) {
                    break;
                }
                Thread.Sleep( 1 );
            }

            float[] values = new[] {
                0.2f,
                0.3f,
                0.4f
            };

            foreach( float value in values ) {

                User user = User
                    .Builder( "foo" )
                    .Custom( attributeName, LdValue.Of( value ) )
                    .Build();

                bool result = client.BoolVariation( "test_key", user, defaultValue: false );
                Console.WriteLine( "{0} => {1}", value, result );
            }
        }
    }
}

Output:

2021-10-19 16:37:21.430 -04:00 [LaunchDarkly.Sdk] INFO: Starting LaunchDarkly client 6.2.2
2021-10-19 16:37:22.042 -04:00 [LaunchDarkly.Sdk] INFO: Waiting up to 10000 milliseconds for LaunchDarkly client to start...
0.2 => True
0.3 => False
0.4 => False

Where 0.3 should be True because lessThanOrEqual to 0.3. Switching to double values fixes it though.