test1

I hit a bug with Optimizely C# SDK when toying in "short lived" console app. The issue is mainly that the SDK does not employ C# async/await best practice "Async All the Way". Reference can be here.

The problematic code is here and the usages are here. And the two scenarios that I came up with are as below:

//Scenario 1
var client = new Optimizely(dataFile);
client.Activate("experimentId", "userId", attrs);
//Activate() will call SendImpressionEvent() which calls EventDispatcher.DispatchEvent(impressionEvent); But Activate() does not have an async method signature, hence not possible to await on the response.

client.Track("Clicks", "userId");
//By the time Track() is called, if above DispatchEvent is still in-flight (not complete), I'm assuming Track() will fail due to "userId" has not been activated for "experimentId".

//Scenario 2
var client = new Optimizely(dataFile);
client.Activate("experimentId", "userId", attrs);
client.Track("Clicks", "userId");
//At this point in time, my console app exits, because Track() does not have an async method signature, hence I'm not able to tell if the DispatchEvent inside Track() has completed successfully ot not.

I'll also submit a PR for fix soon.

Update:

Having had a bit more time to think about this issue, I believe it is best for DispatchEvent() to be sequential and not fire & forget. For fire & forget, it is best to be more explicit in terms of method signature.

Proposed change is here:

https://github.com/HistoricallyCorrect/csharp-sdk/commit/44ffbbfd2598bcd9a60d678b88d1e667d1865eab

I get this at runtime with the latest 2.2 release

Could not load file or assembly 'OptimizelySDK' or one of its dependencies. Strong name signature could not be verified. The assembly may have been tampered with, or it was delay signed but not fully signed with the correct private key. (Exception from HRESULT: 0x80131045)

Similar to how Optimizely implements IOptimizely, OptimizelyUserContext should also implement an interface so that consumers of your SDK can properly mock it for unit testing.

I think it would be a fairly small change.

1. Add an IOptimizelyUserContext interface.
2. Implement that interface in the OptimizelyUserContext class.
3. Update OptimizelyUserContext constructor to accept an IOptimizely instance instead of an Optimizely instance.
4. Update the CreateUserContext method signature to return this new interface rather than the actual type.

All actual implementations remain the same.

Please clarify XML doc comments and/or add Obsolete attributes to indicate methods which should not be used for new projects.

For example, Optimizely.Activate should not be used with a "Flags project which uses the new SDK".

The following docs appear to need updating as well:

• README
• docs under docs/readme-sync/sdk-reference-guides/csharp-sdk

Create a new "Blank App (Universal Windows)" C# UWP app using VS-2019 using default parameters.
add Optimizely.SDK 3.9.1, NuGet package from https://www.nuget.org/packages/Optimizely.SDK

Add some simple calls to initialize SDK. Try to build x86 or x64 give same errors:
Severity Code Description Project File Line Suppression State
Error Could not copy the file "obj\x86\Debug\App.xbf" because it was not found. OptimizelyUWPTest
Error Could not copy the file "obj\x86\Debug\MainPage.xbf" because it was not found. OptimizelyUWPTest
Error Could not copy the file "obj\x86\Debug\OptimizelyUWPTest.xr.xml" because it was not found. OptimizelyUWPTest
Error Cannot resolve Assembly or Windows Metadata file 'Type universe cannot resolve assembly: System.Configuration.ConfigurationManager, Version=4.0.1.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51.' OptimizelyUWPTest C:\Users\angel\ms-src\Scratch\Optimizely\OptimizelyUWPTest\OptimizelyUWPTest\OptimizelyUWPTest.csproj

Note that a C# .NET Core console app does work.

See attached test project.
OptimizelyUWPTest.zip

The following exception handling code in HttpClientEventDispatcher45 only logs the message:

catch (Exception ex)
{
    Logger.Log(LogLevel.ERROR, string.Format("Error Dispatching Event: {0}", ex.Message));
}

Which results in logs like this:

Error Dispatching Event: An error occurred while sending the request.

There is probably a more helpful inner exception that is missing because only ex.Message is being logged.

I think this code should log ex.ToString() instead so that we get the inner exception details.

Consider pulling this source directly into the project or relying on a dll instead if one is available. Since the source is being pulled in that will pull this source into any project that relies on the Optimizely nuget anywhere down stream.

After updating to version 3.11.3 from 3.11.2.1 (which added reading the Last-Modified header) we get error The format of value '05/18/2023 13:35:06 +00:00' is invalid and cannot get the latest configuration.

I was trying to compile with this packages and I saw that there is a sdependency that Xamarin does not use which is the System.Confugration.ConfigurationManager. Should this be added in order to use this package for Xamarin? Or do we have to create bindings for the native packages?

I'm looking at the .NET demo, and understand how the two sort_by_price sort_by_name variants work. To add another variant sort_by_review, I have to modify the current code. Is there any pattern that you have seen being used by .NET server side developers where variant tests can be created without impacting the existing variant tests code?

HttpClientEventDispatcher45 creates a new HttpClient for every request, which goes against the usage guidance for this class. Doing this will cause socket exhaustion under heavy load.

From the .NET framework docs:

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors. Below is an example using HttpClient correctly.

My organization is in the process of upgrading clients and have started seeing this error:

The numeric metric key is not in event tags.

After some investigation, we have traced it down to this function call

Contrast EventTagUtils.GetNumericValue() to EventTagUtils.GetRevenueValue(). Notice that when the revenue key is not present in the event tags, it logs as a debug. I assume the same pattern should be applied to GetNumericValue()

My organization is working around this by simply adding "value":0, to our event tags for now since error level logs will cause issues in our production system.

As a consequence of #33, the HttpClientEventDispatcher45 class has a singleton HttpClient instance that is used for all requests.

For ASP.NET Core 2.2+ applications, this works against the benefits added by new abstractions such as IHttpClientFactory which handles things like connection pooling and honouring DNS TTL values.

If the .NET Standard versions of the SDK added a constructor overload that allowed an HttpClient instance to be provided by the consuming application, such applications could customise the event dispatcher to leverage their own built-in IoC for HttpClient, which would then also leverage any custom HttpMessageHandler instances, such as for logging and monitoring etc.

At present the only way for the integrator to achieve such benefits is to implement a custom IEventDispatcher which is 99% copy-pasted code from the SDK itself.

It is very difficult to write unit tests around our code that makes use of Optimizely, since it provides no interface from which to create a suitable mock. We can make do by creating our own wrapping class, but this gets messy quickly.

Can an IOptimizely interface be added, so that we can use NSubstitute or a similar library to mock the Optimizely client?

GetFeatureVariableJSON is available in Optimizely it would be nice to have this as part of IOptimizely interface. So that we can mock implementation of Optimizely in our unit test cases.

As noted in #51, the SDK does not follow accepted best practices in its use of async/await.

Specifically, the use of Task.Run() to make async calls "fire-and-forget" here:

At the expense of "returning to the caller immediately", this spawns new tasks to perform the HTTP calls.

A better approach would be to add asynchronous methods to the API, say by introducing a new async version of IOptimizely that the Optimizely class could also implement for integrators to opt into using. This would allow integrators to decide whether to make async tasks fire-and-forget or not, as well as allow CancellationTokens to be flowed to the underlying HttpClient calls where appropriate.

From looking at the code, the calls this flowing of async would apply to are those below:

Such changes would allow the SDK to be "async all the way down".

Hi guys, are these dependencies for use of the lib or can they be marked as dev only ?

any help would be appreciated, cheers

<group targetFramework=".NETFramework4.5">
                <dependency id="JsonNet.PrivateSettersContractResolvers.Source" version="0.1.0" />
                <dependency id="murmurhash" version="1.0.0" />
                <dependency id="Newtonsoft.Json" version="9.0.1" />
                <dependency id="NJsonSchema" version="8.30.6304.31883" />
</group>

Some functions are not implemented in the interface, are there plans to fix this?

Example: GetConfiguration().

If I use DI, I can't make that call unless I use Optimizely object directly.

Hi,

I am using NJsonSchema v10.4.6 on my app (due to some new dependency added) and optimizely started throwing the following exception.

System.TypeLoadException: Could not load type 'NJsonSchema.JsonSchema4' from assembly 'NJsonSchema, Version=10.4.6.0, Culture=neutral, PublicKeyToken=c2f9c3bdfae56102'.
  ?, in bool Validator.ValidateJSONSchema(string configJson, string schemaJson)
  ?, in new Optimizely(string datafile, IEventDispatcher eventDispatcher, ILogger logger, IErrorHandler errorHandler, UserProfileService userProfileService, bool skipJsonValidation, EventProcessor eventProcessor)

Is it possible to make optimizely sdk compatible with newer version of NJsonSchema ?

There is a high severity vulnerability in Newtonsoft.Json prior to version 13.0.1. Because of the continued use of NJsonSchema 8.33.6323.36213 which was brought up as an issue previously in issue #284 and closed with a "we might fix this someday" response using this package forces the use of Newtonsoft.Json 12.0.3.