Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why does FlagdProvider.GetProviderName() return No-op Provider #125

Closed
austindrenski opened this issue Jan 8, 2024 · 2 comments · Fixed by #126
Closed

Why does FlagdProvider.GetProviderName() return No-op Provider #125

austindrenski opened this issue Jan 8, 2024 · 2 comments · Fixed by #126

Comments

@austindrenski
Copy link
Member

austindrenski commented Jan 8, 2024

As mentioned in #124, I'm reviewing/evaluating FlagdProvider for an upcoming project, and I ran across what initially looked like a bug, but then I found a unit test that pinned this behavior, so opening this issue to discuss whether:

(A) this is a bug,
(B) the unit test was intended as a placeholder and just never got updated, or
(C) something in between.

Here's the ref sequence that has me stumped:

private readonly Metadata _providerMetadata = new Metadata("flagd Provider");

/// <summary>
/// Get the provider name.
/// </summary>
public static string GetProviderName()
{
return Api.Instance.GetProviderMetadata().Name;
}

[Fact]
public void TestGetProviderName()
{
Assert.Equal("No-op Provider", FlagdProvider.GetProviderName());
}

Here's what I would have expected to see (and am happy to contribute a patch for if you're all in agreement):

diff --git a/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs b/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
index 33d6c28..caaad9a 100644
--- a/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
+++ b/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
@@ -27,10 +27,15 @@ namespace OpenFeature.Contrib.Providers.Flagd
     /// </summary>
     public sealed class FlagdProvider : FeatureProvider
     {
+        /// <summary>
+        /// The provider name as returned in <see cref="GetMetadata"/>.
+        /// </summary>
+        public const string ProviderName = "flagd Provider";
+
         static int EventStreamRetryBaseBackoff = 1;
         private readonly FlagdConfig _config;
         private readonly Service.ServiceClient _client;
-        private readonly Metadata _providerMetadata = new Metadata("flagd Provider");
+        private readonly Metadata _providerMetadata = new Metadata(ProviderName);

         private readonly ICache<string, object> _cache;
         private int _eventStreamRetries;
@@ -116,14 +121,6 @@ namespace OpenFeature.Contrib.Providers.Flagd
         // just for testing, internal but visible in tests
         internal FlagdConfig GetConfig() => _config;

-        /// <summary>
-        /// Get the provider name.
-        /// </summary>
-        public static string GetProviderName()
-        {
-            return Api.Instance.GetProviderMetadata().Name;
-        }
-
         /// <summary>
         ///     Return the metadata associated to this provider.
         /// </summary>
diff --git a/test/OpenFeature.Contrib.Providers.Flagd.Test/FlagdProviderTest.cs b/test/OpenFeature.Contrib.Providers.Flagd.Test/FlagdProviderTest.cs
index 848a257..b73fc07 100644
--- a/test/OpenFeature.Contrib.Providers.Flagd.Test/FlagdProviderTest.cs
+++ b/test/OpenFeature.Contrib.Providers.Flagd.Test/FlagdProviderTest.cs
@@ -65,7 +65,7 @@ namespace OpenFeature.Contrib.Providers.Flagd.Test
         [Fact]
         public void TestGetProviderName()
         {
-            Assert.Equal("No-op Provider", FlagdProvider.GetProviderName());
+            Assert.Equal("flagd Provider", FlagdProvider.ProviderName);
         }

         [Fact]

Background

For context, the usage pattern that I'm trying to setup is to support registering multiple FeatureProvider services as keyed singletons in a dependency container, from which provider-specific clients could then be resolved by provider name:

// client is configured for the FlagdProvider
var client = services.GetRequiredKeyedService<IFeatureClient>(FlagdProvider.GetProviderName());

or callers could resolve a default client configurable based on some TBD criteria (currently just the first named client to be registered):

// client is configured for whichever `unKeyedImplementationFactory` was registered first
var client = services.GetRequiredKeyedService<IFeatureClient>();

Note

I'm eventually planning to offer this upstream as part of a new OpenFeature.Extensions.Hosting package à la OpenTelemetry.Extensions.Hosting, but still working through the realistic use cases and general ergonomics, but just mentioning that so this doesn't seem entirely irrelevant to the project at hand.

public static class OpenFeatureServiceCollectionExtensions
{
    static readonly object SetupLock = new();
    static bool Setup;

    public static IServiceCollection AddOpenFeature(this IServiceCollection services)
    {
        Check.NotNull(services);

        AddOpenFeatureFlagd(services);
        AddOpenFeatureOpenTelemetryHooks(services);

        return services;
    }

    public static IServiceCollection AddOpenFeatureFlagd(this IServiceCollection services)
    {
        Check.NotNull(services);

        TryAddOpenFeatureClient(services, FlagdProvider.GetProviderName(), static x => x.GetRequiredKeyedService<IFeatureClient>(FlagdProvider.GetProviderName()));

        services.TryAddEnumerable(ServiceDescriptor.Singleton<FeatureProvider, FlagdProvider>(static x =>
            x.GetRequiredService<IConfiguration>().GetSection("flagd").GetValue<string?>("endpoint") is [_, ..] endpoint
                ? new FlagdProvider(new Uri(endpoint, UriKind.Absolute))
                : new FlagdProvider()));

        return services;
    }

    public static IServiceCollection AddOpenFeatureOpenTelemetryHooks(this IServiceCollection services)
    {
        Check.NotNull(services);

        services.TryAddEnumerable(ServiceDescriptor.Singleton<Hook, MetricsHook>());
        services.TryAddEnumerable(ServiceDescriptor.Singleton<Hook, TracingHook>());

        return services;
    }

    static void TryAddOpenFeatureClient(IServiceCollection services, string? key, Func<IServiceProvider, IFeatureClient> unKeyedImplementationFactory)
    {
        services.TryAddScoped(unKeyedImplementationFactory);

        services.TryAddKeyedScoped<IFeatureClient>(key, static (services, key) =>
        {
            EnsureSetup(services);

            var current = Api.Instance.GetProvider(key as string);

            var context = EvaluationContext.Builder();

            foreach (var e in services.GetKeyedServices<EvaluationContext>(null))
            {
                context.Merge(e);
            }

            return Api.Instance.GetClient(
                current.GetMetadata().Name,
                null,
                services.GetRequiredService<ILoggerFactory>().CreateLogger(current.GetType()),
                context.Build());
        });

        services.TryAddKeyedSingleton<EvaluationContext>(null, static (x, _) => EvaluationContext
            .Builder()
            .Set("env", x.GetRequiredService<IHostEnvironment>().EnvironmentName)
            .Build());
    }

    static void EnsureSetup(IServiceProvider services)
    {
        if (Setup)
            return;

        lock (SetupLock)
        {
            if (Setup)
                return;

            foreach (var provider in services.GetServices<FeatureProvider>())
            {
                Api.Instance.SetProvider(provider.GetMetadata().Name, provider);
            }

            Api.Instance.AddHooks(services.GetServices<Hook>());

            Setup = true;
        }
    }
}

At present, I have to hardcode flagd Provider in my code to achieve my desired/expected behavior:

- TryAddOpenFeatureClient(services, FlagdProvider.GetProviderName(), static x => x.GetRequiredKeyedService<IFeatureClient>(FlagdProvider.GetProviderName()));
+ TryAddOpenFeatureClient(services, "flagd Provider", static x => x.GetRequiredKeyedService<IFeatureClient>("flagd Provider"));
@toddbaert
Copy link
Member

This is certainly a bug, and just an oversight of some IDE autocomplete or something like that! Please feel free to open a PR with your described fix, and thanks for your close attention. One thing we've been wondering is if it's better to omit "provider" from provider names, since it's redundant (ie: "flagd" instead of "flagd Provider"). Interested in your take on that... we might consider unifying all implementations to use one or the other for a 1.0 release.

I'm eventually planning to offer this upstream as part of a new OpenFeature.Extensions.Hosting package à la OpenTelemetry.Extensions.Hosting, but still working through the realistic use cases and general ergonomics, but just mentioning that so this doesn't seem entirely irrelevant to the project at hand.

This has been suggested before, and I think it's a good idea. I would welcome such a PR, or at least an issue proposing it.

austindrenski added a commit to austindrenski/open-feature-dotnet-sdk-contrib that referenced this issue Jan 8, 2024
austindrenski added a commit to austindrenski/open-feature-dotnet-sdk-contrib that referenced this issue Jan 8, 2024
austindrenski added a commit to austindrenski/open-feature-dotnet-sdk-contrib that referenced this issue Jan 8, 2024
@austindrenski
Copy link
Member Author

austindrenski commented Jan 8, 2024

This is certainly a bug, and just an oversight of some IDE autocomplete or something like that! Please feel free to open a PR with your described fix, and thanks for your close attention.

Awesome, just opened #126 with the minimal patch for now.

One thing we've been wondering is if it's better to omit "provider" from provider names, since it's redundant (ie: "flagd" instead of "flagd Provider"). Interested in your take on that... we might consider unifying all implementations to use one or the other for a 1.0 release.

I would love to see these shortened to whitespace-free provider aliases, i.e. flagd Provider => flagd, and would offer that there's already some well-regarded prior art for this approach via Microsoft.Extensions.Logging:

  • https://learn.microsoft.com/aspnet/core/fundamentals/logging#configure-logging
    {
      "Logging": {
        "LogLevel": { // No provider, LogLevel applies to all the enabled providers.
          "Default": "Error",
          "Microsoft": "Warning",
          "Microsoft.Hosting.Lifetime": "Warning"
        },
        "Debug": { // Debug provider.
          "LogLevel": {
            "Default": "Information" // Overrides preceding LogLevel:Default setting.
          }
        },
        "Console": {
          "IncludeScopes": true,
          "LogLevel": {
            "Microsoft.AspNetCore.Mvc.Razor.Internal": "Warning",
            "Microsoft.AspNetCore.Mvc.Razor.Razor": "Debug",
            "Microsoft.AspNetCore.Mvc.Razor": "Error",
            "Default": "Information"
          }
        },
  • ConsoleLoggerProvider
    /// <summary>
    /// A provider of <see cref="ConsoleLogger"/> instances.
    /// </summary>
    [UnsupportedOSPlatform("browser")]
    [ProviderAlias("Console")]
    public partial class ConsoleLoggerProvider : ILoggerProvider, ISupportExternalScope
  • DebugLoggerProvider
    /// <summary>
    /// The provider for the <see cref="DebugLogger"/>.
    /// </summary>
    [ProviderAlias("Debug")]
    public class DebugLoggerProvider : ILoggerProvider

I'm eventually planning to offer this upstream as part of a new OpenFeature.Extensions.Hosting package à la OpenTelemetry.Extensions.Hosting, but still working through the realistic use cases and general ergonomics, but just mentioning that so this doesn't seem entirely irrelevant to the project at hand.

This has been suggested before, and I think it's a good idea. I would welcome such a PR, or at least an issue proposing it.

I'll open a new issue to track this (see: open-feature/dotnet-sdk#264), and will see about getting a draft PR put together over the next couple of weeks.

austindrenski added a commit to austindrenski/open-feature-dotnet-sdk-contrib that referenced this issue Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants