Updating SDK to 0.5.2 spec #23

Norhaven · 2024-01-22T07:24:08Z

This implements the C# SDK to be compliant with the 0.5.2 version of the GrowthBook SDK spec. Things that are a part of this include:

New unit test structure for easier use of the standard cases.json tests (all tests are currently passing).
New feature repository/cache with support for Server Sent Events, background retrieval, and encrypted features.
Extensive logging which can be enabled by log level (or overridden with a custom implementation).
More robust error handling.

Please let me know if you have any questions.

Thanks!

…work

darklordzw · 2024-01-22T19:10:42Z

@Norhaven, looks great, thanks for contributing such as substantial update! My one question is why you changed all of the double's to floats for things experiment weights? I'm not necessarily opposed to it, but I'm curious as to the reason.

darklordzw · 2024-01-22T19:14:34Z

@Mastermindzh, @pascal89 any thoughts on this PR before I merge this in?

Mastermindzh · 2024-01-22T19:16:47Z

@darklordzw I'll have a look tomorrow morning (CEST) and let you know 😊

Norhaven · 2024-01-22T21:46:30Z

@darklordzw Thanks for taking a look! The use of floats instead of doubles was primarily because it seemed like the typical floating point values needed by things like weights didn't require more than ~7 digits of precision to be effective, but I'm open to either way if there's a preference for doubles in particular. Thoughts?

pascal89 · 2024-01-23T11:56:34Z

@darklordzw I'll schedule a review moment for tomorrow

darklordzw · 2024-01-23T15:05:34Z

@Norhaven, gotcha. My original implementation used floats, but I ended up switching to doubles to be consistent with the other Growthbook sdks (this was originally a port of the Python version). I don't have a strong opinion either way, but I lean towards staying with doubles to stay consistent with what's out there already.

Norhaven · 2024-01-23T18:17:26Z

@darklordzw Sure, makes sense to me. I've replaced the float usages with doubles now. The precision change caused a minor rounding difference in the GetEqualWeights test that differs from the spec, so I've called that out in there with a little more info and adjusted it slightly. Let me know if you have more questions on anything, thanks!

Mastermindzh

Overall it looks good, I made several recommendations though.

Mastermindzh · 2024-01-23T21:02:33Z

GrowthBook/Api/FeatureRefreshWorker.cs

+                                    continue;
+                                }
+
+                                json = json.Substring(5).Trim();


I feel this statement should be inside of the if statement.
Now you'll trim 5 characters no matter the result of the statement :)

Hmm... not sure I follow, but added some more comments to hopefully clarify the reasoning behind doing it this way. Let me know if I missed the point. :)

Mastermindzh · 2024-01-23T21:10:53Z

GrowthBook/Api/HttpClientFactory.cs

+{
+    public class HttpClientFactory : IHttpClientFactory
+    {
+        public static class ConfiguredClients


I would extract this and move it to a separate file since the scope isn't limited to just this file.

Good idea, moved it out now.

Mastermindzh · 2024-01-23T21:17:30Z

GrowthBook.Tests/ApiTests/FeatureRefreshWorkerTests.cs

+
+    private readonly TestHttpClientFactory _httpClientFactory;
+    private readonly GrowthBookConfigurationOptions _config;
+    private readonly FeatureRefreshWorker _worker;


use the new keyword to get rid of the "hiding" errors

Whoops, forgot to remove that from the base class. Thanks!

Mastermindzh · 2024-01-23T21:20:42Z

GrowthBook.Tests/ApiTests/LoadFeaturesTests.cs

+{
+    private const string ApiKey = "<AN API KEY GOES HERE>";
+
+    [Fact(Skip = "This is here as a shortcut for the purpose of debugging this SDK against the actual API and should not be used otherwise")]


I don't like including these things t.b.h

Yeah, I was a little on the fence about that as well. Removed.

Mastermindzh · 2024-01-23T21:21:14Z

GrowthBook.Tests/ApiTests/LoadFeaturesTests.cs

+
+public class LoadFeaturesTests
+{
+    private const string ApiKey = "<AN API KEY GOES HERE>";


If we're including this file I'd recommend using IConfiguration for these things:
See, this for an example.

File is removed now.

Mastermindzh · 2024-01-23T21:27:59Z

GrowthBook/AssemblyInfo.cs

@@ -0,0 +1,6 @@
+using System.Runtime.CompilerServices;


I suggest removing this file altogether and stick with the portable configuration in csproj:

<ItemGroup> <InternalsVisibleTo Include="GrowthBook.Tests" /> </ItemGroup>

Good idea, forgot about that. Done.

Mastermindzh · 2024-01-23T21:30:39Z

GrowthBook/Extensions/ValidationExtensions.cs

+        /// </remarks>
+        /// <param name="value">The string to verify.</param>
+        /// <returns>True if null, empty, or whitespace, false otherwise.</returns>
+        public static bool IsMissing(this string value) => string.IsNullOrWhiteSpace(value);


To me, this obfuscated what we're doing when calling IsMissing instead of providing clarity.

Ahh, gotcha. Was trying to remove some of the boilerplate and focus more on the semantics of what the caller is trying to achieve, but looks like it missed the mark. Will take another look at that, thanks.

Mastermindzh · 2024-01-23T21:31:52Z

GrowthBook/FeatureResult.cs

+        /// <summary>
+        /// Represents possible values for the source of a feature result.
+        /// </summary>
+        public static class SourceIs


I would recommend removing since it isn't used.
Also, I think you meant "SourceIDs"

It's actually used when calling GetFeatureResult and was intended to be a more fluent-property kind of thing so it reads FeatureResult.SourceIs.DefaultValue or similar. Do you feel that it's more confusing that way?

Actually, the more I think about it, we don't currently have much of a fluent-syntax usage in this SDK, would add to the confusion. Renamed.

Mastermindzh · 2024-01-23T21:35:11Z

GrowthBook/GrowthBook.csproj

@@ -2,7 +2,7 @@

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
-    <Authors>Ben Whatley</Authors>
+    <Authors>Ben Whatley,Norhaven</Authors>


I would usually only put recognized contributors of the Github repository here

Fair enough, removed. :)

Mastermindzh · 2024-01-23T21:35:28Z

GrowthBook/GrowthBook.csproj

@@ -11,11 +11,17 @@
    <RepositoryUrl>https://github.com/growthbook/growthbook-csharp.git</RepositoryUrl>
    <RepositoryType>git</RepositoryType>
    <PackageTags>GrowthBook,A/B test,feature toggle</PackageTags>


Would recommend to include "flag"

Norhaven · 2024-01-24T18:04:11Z

@Mastermindzh Thanks for the review, updated the PR and had a few questions.

pascal89

I didn't review the test cases due to a lack of time.
I really like the infrastructure for keeping the features up-to-date (with the server side events) 👍 👍. It would be nice to have some usage documentation in the README.md, as any local implementation of this package will probably be impacted because of these changes. Also, I'm not sure how to work with attributes. We are using this package in a web API and for each HTTP request I need to set the attributes in order to target features to a certain audience (user or user role). Could you shed some light on this?, preferably in the form of documentation.

pascal89 · 2024-01-24T19:33:59Z

GrowthBook/Api/InMemoryFeatureCache.cs

+            }
+        }
+
+        public async Task RefreshWith(IDictionary<string, Feature> features, CancellationToken? cancellationToken = null)


This doesn't look like an async function, or am I missing something?

I made that call return a Task in the IGrowthBookFeatureCache interface that this implements because, assuming usage is backend in a distributed environment (e.g. cloud), people may want to provide their own cache that works better for their situation, like a Redis-backed one or similar, instead of just an in-memory one. That kind of thing is likely to be more appropriately accessed in an async manner, so I preemptively went that direction.

And you're right, the InMemoryFeatureCache itself has no async needs, but it should probably conform to that pattern anyway. I made a quick change now to remove the 'async' keyword and just explicitly return a Task.CompletedTask to make things a little more clear and avoid the warning, hopefully that all helps. :)

pascal89 · 2024-01-24T19:39:20Z

GrowthBook/Experiment.cs

@@ -26,7 +26,7 @@ public class Experiment
        /// <summary>
        /// How to weight traffic between variations. Must add to 1.
        /// </summary>
-        public IList<double> Weights { get; set; }
+        public List<double> Weights { get; set; }


I'd always go for an interface (such as IEnumerable<> or IList<>) over a concrete type. That way you're more flexible in your implementation. I also doubt you need a List for this, as these properties come from GrowthBook you probably only need to iterate over this collection.

I'd agree. Hmm... from my perspective, I would tend to prefer an array in here on data objects just so we can both have explicit length checks/indexing available as well as enforce the readonly semantics a little more to discourage adding/removing items, but with the existing SDK implementation using lists I wasn't sure if there was a use case for people wanting to add their own values on the fly to these objects. Taking a breaking change to that might cause problems.

@darklordzw Just wondering, did you have a particular design decision in mind choosing lists over something else for the existing implementation? Might help inform the directions we can go here. :)

@Norhaven, I tend to prefer ILists over arrays in most cases unless there's a reason to go with arrays. I forget why I went with a list over IEnumerable originally, I suspect it was another case of wanting to stay syntactically consistent with the other SDKs, which also use indexable lists.

pascal89 · 2024-01-24T20:14:22Z

GrowthBook/GrowthBook.cs

-        /// <param name="experiment">The experiment to evaluate.</param>
-        /// <returns>The experiment result.</returns>
-        ExperimentResult RunExperiment(Experiment experiment)
+        public async Task LoadFeatures(GrowthBookRetrievalOptions options = null, CancellationToken? cancellationToken = null)


/// <inheritdoc /> You want documentation on your public methods ;-)

Good point, added. :)

pascal89 · 2024-01-25T08:20:35Z

GrowthBook/Api/FeatureRefreshWorker.cs

+
+            if (_config.PreferServerSentEvents)
+            {
+                _isServerSentEventsEnabled = response.Headers.TryGetValues("x-sse-support", out var values) && values.Contains("enabled");


I saw this constant more than once in the code base. Maybe you could make it a constant.

Sounds good, done.

pascal89 · 2024-01-25T08:25:15Z

GrowthBook/Api/FeatureRefreshWorker.cs

+            var hostEndpoint = config.ApiHost;
+            var trimmedHostEndpoint = new string(hostEndpoint?.Reverse().SkipWhile(x => x == '/').Reverse().ToArray());
+
+            _featuresApiEndpoint = $"{trimmedHostEndpoint}/api/features/{config.ClientKey}";


If you'd used a typed http client, you could have contained all the logic related to communicating with the GrowthBook Api. Then this service worker would only contain the logic in keeping the GrowthBookFeatureCache up to date.

Ahh, gotcha. Will look into that, thanks!

pascal89 · 2024-01-25T08:28:22Z

GrowthBook/GrowthBook.cs

+            var featureCache = new InMemoryFeatureCache(cacheExpirationInSeconds: 60);
+            var httpClientFactory = new HttpClientFactory(requestTimeoutInSeconds: 60);
+
+            var loggerFactory = context.DefaultLoggerFactory ?? LoggerFactory.Create(builder =>


Isn't this a concern of the consumer of this package to configure?

Yeah, I think you're right. I'd added in the console/debug logging for testing and provided the DefaultLogLevel property on the Context that could modify this, but looking at it some more I think that's an extraneous property and this can be condensed and not force a particular logging provider on the user. Will fix that. Thanks!

Norhaven · 2024-01-29T03:06:49Z

@darklordzw @Mastermindzh @pascal89 Okay, the PR is ready for another look over, please let me know if you have questions. I've left the existing comments open for any further discussion necessary, feel free to resolve them if it works for you. Thanks!

Mastermindzh · 2024-01-29T09:44:06Z

@Norhaven @darklordzw

All my feedback has been implemented and then some. I don't see a reason not to merge this 😄.
Great job @Norhaven !

pascal89 · 2024-01-29T13:38:45Z

@Norhaven Great work! Still have a little remark though. It would be nice if you could include two additions:

Update the README.md because setting up GrowthBook in any implementation has changed a bit because of the infrastructure for fetching the features and keeping it up-to-date is now included in the package.
Update the CHANGELOG.md to include the changes you've made.

Norhaven · 2024-01-29T17:43:13Z

@pascal89 Just updated the changelog now, thanks! For the readme and documentation, I also have an extensive ASP.Net Core backend example demonstrating the basic use cases and how to set things up with this version of the SDK that will go into the https://github.com/growthbook/examples repo once this PR is complete and out to NuGet.

I'm thinking that once the example PR is approved and merged over there then I can link it in the readme over here, would that be sufficient or do you think that it would still help to put some basic 'getting started' info here as well?

pascal89 · 2024-01-29T19:05:06Z

I'm glad you've worked on some examples as well 😄 As long as we'll link to the example(s) from the readme it's fine for me. If you want my opinion on the examples you'll be adding, feel free to request me as a reviewer. I think that accounts for @Mastermindzh as well.
For this PR, IMO It's ready to go.

darklordzw · 2024-01-29T19:21:14Z

PR has been merged into main. Thanks again for your contributions @Norhaven! I'll have a version up on NuGet in a few.

darklordzw · 2024-01-29T19:38:15Z

v1.0.0 has is live on NuGet now.

Norhaven added 9 commits December 28, 2023 16:21

Updated data structure, added unimplemented functionality

d8bb875

Finished spec methods/logic, added most standard tests and test frame…

a1025e8

…work

Added decryption/version compare and fixed last tests

41888d7

Refactored for clarity, fixed tests

12776f0

Reorganized for clarity, added initial API integration

fafdf68

Added API unit tests, refactored to fix small issues

93d8c7e

Added XML docs, comments, logging where necessary

56e6e7c

Minor refactoring

3c0caff

Updated project properties

4bc4c0d

Replaced usages of float with double, adjusted GetEqualWeights test

9623452

Mastermindzh reviewed Jan 23, 2024

View reviewed changes

Made changes from PR feedback

4cc1732

PR fixes

a182c6e

pascal89 reviewed Jan 25, 2024

View reviewed changes

Norhaven added 6 commits January 25, 2024 11:56

Various PR fixes

b96558c

Changed to IList in data models

880b499

Refactored to pull HTTP logic into extensions

f374527

Fixed defaulting of feature repository

e5b2021

Replaced Moq with NSubstitute

a4cf071

Fixing brittleness of feature refresh worker test

d393950

Updated changelog

b6fb94c

pascal89 approved these changes Jan 29, 2024

View reviewed changes

darklordzw merged commit 57b6b0b into main Jan 29, 2024
3 checks passed

Norhaven deleted the fully-updating-sdk branch February 2, 2024 23:14

Updating SDK to 0.5.2 spec #23

Updating SDK to 0.5.2 spec #23

Conversation

Norhaven commented Jan 22, 2024

darklordzw commented Jan 22, 2024

darklordzw commented Jan 22, 2024

Mastermindzh commented Jan 22, 2024

Norhaven commented Jan 22, 2024 • edited Loading

pascal89 commented Jan 23, 2024

darklordzw commented Jan 23, 2024

Norhaven commented Jan 23, 2024

Mastermindzh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Norhaven Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Norhaven commented Jan 24, 2024

pascal89 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Norhaven commented Jan 29, 2024

Mastermindzh commented Jan 29, 2024

pascal89 commented Jan 29, 2024 • edited Loading

Norhaven commented Jan 29, 2024 • edited Loading

pascal89 commented Jan 29, 2024

darklordzw commented Jan 29, 2024

darklordzw commented Jan 29, 2024

Norhaven commented Jan 22, 2024 •

edited

Loading

Norhaven Jan 25, 2024 •

edited

Loading

pascal89 left a comment •

edited

Loading

pascal89 commented Jan 29, 2024 •

edited

Loading

Norhaven commented Jan 29, 2024 •

edited

Loading