chore: yggdrasil #224

nunogois · 2024-06-26T15:04:32Z

https://linear.app/unleash/issue/2-1545/yggdrasil-net-integrate-into-unleashclient-net-sdk

This incorporates Yggdrasil into the .NET SDK, using the new Unleash.Yggdrasil NuGet package.

The PR tries to target as small of a change surface as possible, however some things simply no longer made sense or needed to change to accommodate this change to the new Yggdrasil engine.

After this change we can consider removing a big part of what can now be considered "dead code". There are a lot of refactoring opportunities. We should also consider favoring state literal strings in tests over constructing the state with the current classes. However all of this is out of scope for this PR.

Some changes that affect the public interface and usability of the SDK include:

Due to Unleash.Yggdrasil's target of .NET Standard 2.0, we're dropping support for:
- .NET Framework 4.7;
- .NET Framework 4.6.1;
- .NET Framework 4.6;
- .NET Framework 4.5.1;
- .NET Framework 4.5;
In the Unleash client, we no longer expose:
- FeatureToggles;
- GetVariants;
IStrategy -> Yggdrasil.IStrategy at Unleash client creation, for custom strategies;
IToggleBootstrapProvider.Read() should now return a state string instead of a ToggleCollection;
UnleashServices now logs an error and emits a FileCache error whenever YggdrasilEngine fails to load initial state;

Some changes that are internal to the SDK include:

FetchTogglesResult drops ToggleCollection in favor of State;
SendMetrics now takes in an Yggdrasil.MetricsBucket instead of a ThreadSafeMetricsBucket;
CachedFilesLoader no longer needs a serializer, since it uses the file string content as is;
CachedFilesLoader drops InitialToggleCollection in favor of InitialState;
UnleashServices now holds a list of DefaultStrategyNames in order to register them as part of metrics;
UnleashServices no longer initializes a ToggleCollection and MetricsBucket. Instead, it initializes an YggdrasilEngine;
Variant now inherits from Yggdrasil.Variant;
Payload now inherits from Yggdrasil.Payload;
UnleashContext now inherits from Yggdrasil.Context;
ClientMetricsBackgroundTask drops ThreadSafeMetricsBucket in favor of YggdrasilEngine;
FetchFeatureTogglesTask drops ThreadSafeToggleCollection and IJsonSerializer in favor of YggdrasilEngine;
Due to the simplification of some processes, like (de)serialization of state, some tests were simplified, adapted or removed. They can probably be simplified further on a later PR;

We also fixed some SDK behaviors that followed wrong assumptions.

For more details, feel free to ask or check the diff directly.

src/Unleash/DefaultUnleash.cs

src/Unleash/Internal/UnleashServices.cs

sighphyre

This is looking sexy!

sighphyre

I'm happy. Few optional thoughts, mostly for others who may not have context coming into this PR.
I'm okay accepting this in it's current state, looks good! I do think we should wait for one more set of eyes though
Definitely worth another PR to get the full removal of the legacy business logic objects but let's merge this first. Great job!

sighphyre · 2024-06-28T13:30:46Z

src/Unleash/Communication/UnleashApiClient.cs

-                    Bucket = bucket
-                });
-            }
+                clientRequestHeaders.AppName,


Don't really mind if you wanna keep it but this used to be a named field and now it's not. Intentional?

Not going to affect the serialized outcome. But we should be consistent, we're using
InstanceId = clientRequestHeaders.InstanceTag, the row below so we should pick one and stick to it

My editor complained about this one, due it to being redundant. I assume because in AppName = clientRequestHeaders.AppName, AppName can be deducted as the end property name, and as such it can be dropped.

That would not be the case with InstanceId = clientRequestHeaders.InstanceTag for example, since InstanceId != InstanceTag.

I'm OK with reverting this but I don't think it matters much 🤷

My editor complained about this one

I don't think it matters much 🤷

Yuh, happy for you to leave it alone if it's a tool complaining

Fair point, I'm not going to be too fussy about it either then :)

sighphyre · 2024-06-28T13:32:36Z

src/Unleash/Internal/CachedFilesLoader.cs

@@ -72,7 +68,7 @@ public CachedFilesResult EnsureExistsAndLoad()
                try
                {
                    fileSystem.WriteAllText(toggleFile, string.Empty);
-                    result.InitialToggleCollection = null;
+                    result.InitialState = string.Empty;


For other reviewers - looks super suspect but this is really just to get a test to shush. All internal so I don't think it's a biggie

sighphyre · 2024-06-28T13:36:14Z

src/Unleash/Scheduling/ClientMetricsBackgroundTask.cs

        }

        public async Task ExecuteAsync(CancellationToken cancellationToken)
        {
            if (settings.SendMetricsInterval == null)
                return;

-            var result = await apiClient.SendMetrics(metricsBucket, cancellationToken).ConfigureAwait(false);
+            var result = await apiClient.SendMetrics(engine.GetMetrics(), cancellationToken).ConfigureAwait(false);


I think this is okay for now, we'll probably have to do another pass later in a different PR to deal with the fact that this completely drops metrics if the HTTP call borks - the GetMetrics call to Ygg is destructive in that it clears the metrics once they've been read.

Might be a "Metrics Extensions v3" problem

Any chance this is related to https://github.com/Unleash/unleash-client-dotnet/pull/224/files#r1660524625?

Kinda sorta. It's adjacent certainly. But I'll put details on that comment rather. This can still wait, the other we should talk about

sighphyre · 2024-06-28T13:40:32Z

src/Unleash/Scheduling/FetchFeatureTogglesTask.cs

-            toggleCollection.Instance = result.ToggleCollection;
+            if (!string.IsNullOrEmpty(result.State))
+            {
+                engine.TakeState(result.State);


@daveleek This raises a new exception type, any idea where Exceptions that leave this method go? If they land in caller space then I'd like to wrap and reraise. If they get swallowed by async mechanics and become an error event then I don't care that much

The primary use case is fine, gets called from the scheduler and outputs an error message in a log output. But this ExecuteAsync also gets called from UnleashClientFactory.CreateClient if synchronousInitialization == true, where errors are going straight back to caller space

Is that intended? What should our plan be here?

The primary use case is fine, gets called from the scheduler and outputs an error message in a log output. But this ExecuteAsync also gets called from UnleashClientFactory.CreateClient if synchronousInitialization == true, where errors are going straight back to caller space

If I'm reading that correctly then yes, lands in user space

@nunogois sounds like

engine.TakeState(result.State); } catch(YggException ex){ throw new UnleashException(ex) }

or something very close is the correct thing here. Effectively what I don't want to happen here is code in the wild that's only catching UnleashException to have an YgggrasilException blow past it and unwind someone's stack

Thanks! How about something like this? 29650ef - Should be consistent with how other exceptions are handled in this file.

Awesome! Thanks @nunogois 👌

sighphyre · 2024-06-28T13:41:59Z

src/Unleash/Unleash.csproj

@@ -16,31 +16,15 @@
  </PropertyGroup>

  <!-- Need to conditionally bring in references for the .NET Framework 4.* targets -->
- <ItemGroup Condition="'$(TargetFramework)' == 'net45'">


For other reviewers - we decided to drop older versions of .NET here. They're EOL, becoming burdensome to maintain and are blocking for the Yggdrasil wrapper

Great! We should be explicit about this and release a major version.
Seeing as we're publishing a .NET Standard 2.0 theoretically we indirectly still support down to 4.6.2

Correct, however like the doc you shared mentions:

While NuGet considers .NET Framework 4.6.1 as supporting .NET Standard 1.5 through 2.0, there are several issues with consuming .NET Standard libraries that were built for those versions from .NET Framework 4.6.1 projects. For .NET Framework projects that need to use such libraries, we recommend that you upgrade the project to target .NET Framework 4.7.2 or higher.

So we made the decision to drop these.

@daveleek If I read @nunogois and the linked doc correctly (and I only think I do) then 4.6.2 is out. I'm open to being told we need 4.6.2 but burdensome is what I want to call that and I'm very lazy

No I'm fine with them being dropped by us which is what I'm saying in a roundabout way. With Standard 2.0 we're supporting a few runtimes (through using Standard 2.0) even if we drop the explicit support for them, in the cases where that is absolutely necessary. But yes, people on 4.6.1 might experience issues trying to add our library to their projects

sighphyre · 2024-06-28T13:44:56Z

tests/Unleash.Tests/Internal/Dependent_Features_Tests.cs

-            childMetrics.Yes.Should().Be(1L);
+            var bucket = unleash.services.engine.GetMetrics();
+            var childMetrics = bucket?.Toggles.Single(t => t.Key == "child-1").Value;
+            childMetrics?.No.Should().Be(0L);


Oh that's very subtle, can we assert not null here? My eyes are telling me (perhaps wrong) that the asserts won't run if metrics is null

Yeah +1 on that

Do you mean something like this? e7752b4

Unfortunately my editor type checker doesn't care about the assertion and still complains that bucket could possibly be null, but that's probably okay.

bucket.Should().NotBeNull();

Yuh. Happy

Yes, happy!

sighphyre · 2024-06-28T13:46:57Z

tests/Unleash.Tests/DefaultUnleashTests.cs

@@ -132,7 +132,13 @@ public static IUnleash CreateUnleash(string name, ToggleCollection state)
            var httpClient = new HttpClient(fakeHttpMessageHandler) { BaseAddress = new Uri("http://localhost") };
            var fakeScheduler = A.Fake<IUnleashScheduledTaskManager>();
            var fakeFileSystem = new MockFileSystem();
-            var toggleState = Newtonsoft.Json.JsonConvert.SerializeObject(state);
+            var toggleState = Newtonsoft.Json.JsonConvert.SerializeObject(state, new Newtonsoft.Json.JsonSerializerSettings


For other reviewers: this is a very clever little trick, compliments of @nunogois.

The existing state in the tests is now serialised and passed like that. This allows us to preserve the bulk of the tests with minimal changes and have confidence that we don't do too much surgery on the tests while we're also changing the code

The serialiser change here is to get .NET to send camelCased Json rather than PascalCased JSON, which Yggdrasil doesn'tt like

If making a major (semver) change for this release, why not go to System.Text.Json, which would then allow for further non-breaking work to adopt JSON source generators.

Yep. we chatted, it's a good idea, we're gonna go all in on System.Text.Json. Not in this PR though, this one's already too big

daveleek · 2024-07-01T06:19:06Z

src/Unleash/Communication/UnleashApiClient.cs

-            if (metricsRequestsToSkip > metricsRequestsSkipped)
-            {
-                metricsRequestsSkipped++;
-                return false;
-            }
-
-            metricsRequestsSkipped = 0;


Why are we removing this?

I have no idea what this is, but I saw it dropped in the new method you added in #179 so I thought it was no longer relevant. Do we still care about this? Is this something for the new metrics iteration? Cc: @sighphyre

Ah, yeah, we should probably keep this for now. This is the HTTP backoff stuff we added to stop hammering Unleash when it's not ready to handle a ton of traffic I think this is dropped in #179 because that's quite old (better to say 179 didn't know that it needed it yet because it didn't exist).

This probably needs to change at some point in the near future but this might also be a metrics v3 problem

Thanks for clarifying. 61ebe17 brings this back for now.

daveleek · 2024-07-01T06:22:14Z

src/Unleash/DefaultUnleash.cs

@@ -81,9 +49,6 @@ public DefaultUnleash(UnleashSettings settings, bool overrideDefaultStrategies,
            }
        }

-        /// <inheritdoc />
-        public ICollection<FeatureToggle> FeatureToggles => services.ToggleCollection.Instance.Features;


Removing this is a breaking change, so should perhaps be pointed out in a major release. It's not in the documentation though, so nothing to update there

Yep, noted this in the PR description, and we're planning on a major release. Something that me and @sighphyre discussed is that we'll probably need to prepare a migration guide or similar, but out of scope for this PR.

daveleek · 2024-07-01T06:25:46Z

src/Unleash/IUnleash.cs

-        IEnumerable<VariantDefinition> GetVariants(string toggleName);
-
-        /// <summary>
-        /// Get available feature variants.
-        /// </summary>
-        /// <param name="toggleName">The name of the toggle.</param>
-        /// /// <param name="context">The Unleash context to evaluate the toggle state against.</param>
-        /// <returns>A list of available variants.</returns>
-        IEnumerable<VariantDefinition> GetVariants(string toggleName, UnleashContext context);


These aren't in the documentation either, but also warrants a major release

Yep, same as https://github.com/Unleash/unleash-client-dotnet/pull/224/files#r1661225276

daveleek · 2024-07-01T06:30:12Z

src/Unleash/Internal/Variant.cs

@@ -2,21 +2,18 @@

 namespace Unleash.Internal
 {
-    public class Variant
+    public class Variant: Yggdrasil.Variant


What do we need to keep this one around for? I can't remember

Well, Variant is a public class so I assume we still want it available. If we don't I'm more than happy to drop this entirely and just rely on Yggdrasil.Variant internally. WDYT?

What do we need to keep this one around for? I can't remember

In theory, it stops people having to run around like crazy people updating their namespaces. Just makes this upgrade a slightly less bitter pill to swallow

Ok if you're happy keeping it around then sure. It could be part of the migration article mentioned above though, since they're already having to do a bunch of work

daveleek · 2024-07-01T06:38:58Z

src/Unleash/Variants/VariantPayload.cs

 namespace Unleash.Variants
 {
-    public class Payload
+    public class Payload: Yggdrasil.Payload


Just wondering again why we have to keep this one around, I can't remember the exact reason

Same as https://github.com/Unleash/unleash-client-dotnet/pull/224/files#r1661227880 perhaps?

daveleek · 2024-07-01T06:44:28Z

tests/Unleash.Tests/Utilities/ToggleBootstrapFileProvider_Tests.cs

+            deserializedResult?.Features.Count().Should().Be(3);
+            deserializedResult?.Features.Single(f => f.Name == "featureY").Enabled.Should().Be(false);


Guess that goes for this one too, and the other test further down?

What are these comments in reference to? 😄

Oh you mean null assertion on the deserializedResult?

Let me know if this is better: fe0f1cc

Apologies for lack of context there, but yes! and thanks!

daveleek · 2024-07-01T06:44:48Z

tests/Unleash.Tests/Utilities/ToggleBootstrapUrlProvider_Tests.cs

@@ -36,7 +31,8 @@ public void Gets_The_File_Content()
            var responseContent = bootstrapUrlProvider.Read();

            // Assert
-            responseContent.Features.Should().BeEmpty();
+            var deserializedResponseContent = JsonSerializer.Deserialize<ToggleCollection>(responseContent);
+            deserializedResponseContent?.Features.Should().BeEmpty();


I assume this is the same as #224 (comment), addressed in the same commit: fe0f1cc

daveleek

Awesome work, couple of +1's around some of the changes in tests, and a few comments on how we should communicate some of the contract changes

daveleek

Awesome work @nunogois!

nunogois · 2024-07-11T13:46:59Z

Rebased this and cleaned it up a bit.
@sighphyre let me know when you think it's a good time to merge this. I suggest taking another look just in case 😄

Co-authored-by: Simon Hornby <liquidwicked64@gmail.com>

…id json

daveleek · 2024-08-23T09:58:26Z

src/Unleash/DefaultUnleash.cs

        ///// <param name="strategies">Custom strategies.</param>
-        public DefaultUnleash(UnleashSettings settings, bool overrideDefaultStrategies, params IStrategy[] strategies)


Breaking change here

daveleek · 2024-08-23T09:58:54Z

src/Unleash/DefaultUnleash.cs

-        ///// </summary>
-        ///// <param name="config">Unleash settings</param>
-        ///// <param name="strategies">Additional custom strategies.</param>
-        public DefaultUnleash(UnleashSettings settings, params IStrategy[] strategies)


Replacing this with an Ygg strategy list is a breaking change

daveleek · 2024-08-26T06:07:01Z

src/Unleash/Internal/IToggleBootstrapProvider.cs

 {
    public interface IToggleBootstrapProvider
    {
-        ToggleCollection Read();
+        string Read();


Breaking change

Let's change the name of this one completely. Also now has a deprecation notice in main

rcollette · 2024-09-25T16:57:25Z

What architectures are going to be supported in docker containers? I've been getting burned by rust/go .NET integrations lately where the producing teams do not have access to ARM64 platforms on which to build and code that runs on my local machine, fails to build in a docker on ARM64 Macs.

sighphyre · 2024-09-26T06:49:41Z

What architectures are going to be supported in docker containers? I've been getting burned by rust/go .NET integrations lately where the producing teams do not have access to ARM64 platforms on which to build and code that runs on my local machine, fails to build in a docker on ARM64 Macs.

Out the box, likely x86 Windows/MacOS/Linux + ARM MacOS. Right now there's no Github runners for ARM Windows/Linux, not a blocker but we likely won't go through the dance of setting up a custom runner unless someone asks

Edit: Okay Linux ARM builds will be in by default too, probably

nunogois requested review from daveleek and sighphyre June 26, 2024 15:04