autofac / Autofac.ServiceFabric

Autofac integration for Azure Service Fabric. Provides service factory implementations for Actors, Stateful Services and Stateless Services.
MIT License
26 stars 27 forks source link

Allow injecting additional registrations during the creation of service lifetimescope #57

Open n3gwave opened 4 years ago

n3gwave commented 4 years ago

Problem Statement

Service registration is closed for extension and makes injecting additional dependencies within the SF service lifetime scope impossible. There is a challenge with proper initializing the Application Insights with Autofac. Based on AI for SF docu (https://github.com/microsoft/ApplicationInsights-ServiceFabric) there is a requirement to register FabricTelemetryInitializerExtension.CreateFabricTelemetryInitializer(ServiceContext) as a ITelemetryInitializer. However, this must be done while creating the lifetimescope in RegisterServiceAsync method. Otherwise, we don't have ServiceContext available.

Desired Solution

This can be easily achieved by adding additional parameter of type Action to RegisterStatefulServiceFactory/RegisterStatelessServiceFactory/RegisterActorFactory methods and make sure the parameter is passed from AutofacServiceExtensions class methods. Then in factory methods we can add custom dependencies in Begin scope action

var lifetimeScope = container.BeginLifetimeScope(tag, builder =>
                {
                    builder.RegisterInstance(context)
                        .As<StatefulServiceContext>()
                        .As<ServiceContext>();

                    customRegistrations?.Invoke(builder);
                });

Then we could register service with following code:

                Action<ContainerBuilder> configure = b =>
                {
                    AppInsightsSupportInjectAction(b);

                    b.RegisterInstance(new StateManagerHolder());
                    b.Register(ctx => ctx.Resolve<StateManagerHolder>().StateManager ?? new ReliableStateManager(resolve(ctx)))
                        .As<IReliableStateManager>().InstancePerDependency();
                };

                try
                {
                    containerBuilder.RegisterStatefulService<TService>(serviceTypeName, serviceTypeName, configure => configure.Register(ctx => FabricTelemetryInitializerExtension.CreateFabricTelemetryInitializer(ctx.Resolve<ServiceContext>()))

Alternatives You've Considered

I haven't found any workaround to initialize the SF telemetry initializer in the scope created for SF service. I cannot do it in the Kestrel Webhost, as I have remoting methods (my service implements the SF's IService interface).

Additional Context

In my solution, I use a mix of HostBulder with Autofac along with Autofac SF integration so registering FabricTelemetryInitializer out of service scope ends with exception as serviceContext is not resolved during the Build operation on HostBuilder. The reason for that is the AI integration which needs to resolve ILogger thus it pulls for all ITelemetryInitializers as well.

Additional benefit is that we could also register the IReliableStateManager. Now it's not possible because state manager is properly created withing the StatefulService and lifetime scope registration is being done befor. However with some helper class:

public class StateManagerHolder
    {
        /// <summary>
        /// Gets or sets the state manager.
        /// </summary>
        public IReliableStateManager StateManager { get; set; }
    }

we can inject additional registration

Action<ContainerBuilder> configure = b =>
                {
                    b.RegisterInstance(new StateManagerHolder());
                    b.Register(ctx => ctx.Resolve<StateManagerHolder>().StateManager ?? new ReliableStateManager(resolve(ctx)))
                        .As<IReliableStateManager>().InstancePerDependency();
                };

and make sure we update reference StateManager in RunAsync as follows:

            var holder = this.rootScope.Resolve<StateManagerHolder>();
            if (holder.StateManager == null)
            {
                holder.StateManager = this.StateManager;
            }

the last step is required as for some reason new ReliableStateManager(resolve(ctx)) doesn't create valid reliable manager object - at least in my case injected stateful context in ctor is not assigned properly.

alistairjevans commented 4 years ago

Hey; we're a big bogged down getting the Autofac v6 release out right now.

To be honest, I'm not 100% clear what is being asked here; @alexmg, can you offer any insight?

alexmg commented 4 years ago

Thanks for reporting this issue @n3gwave.

I have added a global hook that allows registrations to be added to the lifetime scope created for the service instance. This is provided to the RegisterServiceFabricSupport method the same as the global constructor exception callback.

The registration should look something like the example below. The ServiceContext is resolved from the IComponentContext because the instance is registered just prior to the callback being invoked while the lifetime scope is being created.

builder.RegisterServiceFabricSupport(
    configurationAction: b =>
        b.Register(c => FabricTelemetryInitializerExtension.CreateFabricTelemetryInitializer(
                c.Resolve<ServiceContext>()))
            .As<ITelemetryInitializer>()
            .SingleInstance());

I'm not sure if the timing of this will be adequate for the WebHostBuilder scenario because the ITelemetryInitializer will not be registered in the root lifetime scope and will not be added until the initial container build. It would be great if you could test this out from the MyGet package feed and let us know if it covers the required scenarios.

Install-Package Autofac.ServiceFabric -Version 4.0.0-develop-00076 -Source https://www.myget.org/F/autofac/api/v3/index.json

If there are issues it would be great to have an example application that could be used for testing. I was not able to test this with the Service Fabric demo application because it has not been updated to include .NET Core examples. It would also be good to have the demo application include the configuration of Application Insights telemetry, but I don't have time to get to this now.

@alistairjevans @alsami @tillig Please hold off releasing the 4.0.0 package until this has been resolved. I have reopened this issue even though there is a commit in play because I'm not convinced it will be enough.

n3gwave commented 4 years ago

Hey, I think that should be fine at least for things I'm doing. You are right that for case with WebHostBuilder and default template this would still be problem, but I use a combination of generic host with Autofac DI and then for services that needs http I create my own http communication listener (forked from kestrel) but has very important detail: UseServiceProviderFactory(new AutofacChildLifetimeScopeServiceProviderFactory(this.rootScope.BeginLifetimeScope(endpointName))) So the listener http web host has child container. That's why it's fine for me to register FabricTelemetryInitializer in the root. It will be passed down to inner hosts as well. But I doubt it people are using SF in the same way. Anyway, it would be also good to support IReliableStateManager by default as well. Currently I'm doing some workarounds and it works but still it's not obvious. Nonetheless, I think your solution could be used with this case as well but with some 'if's' because we need to make sure we are in stateful service and this callback is added globally. I wish I could help with testing it, but unfortunately I've no time for that right now. Basically i'm quite good with workaround I've, at least for now.

alexmg commented 4 years ago

Hi @n3gwave. It would be great if you could use the package from MyGet to confirm that this does address your issue. If everything looks good, we can release the package with the changes.