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

RFC: Add ServiceProviderAccessor to allow access to IServiceProvider from Interceptor #364

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tdg5
Copy link

@tdg5 tdg5 commented Nov 5, 2024

What was changed

This PR is a sketch of a solution for #363 that leverages a pattern that's used in ASP.NET for managing access to the HttpContext instance that is appropriate for a given execution context.

I don't think this PR is done, but I wanted to sketch something for discussion before putting more effort in.

Some things to note:

  • The default execution path is unchanged. Alternate code paths are only executed if a given application registers an IServiceProviderAccessor instance with the IServiceCollection.
  • This is still convoluted, but it does make accessing the scoped IServiceProvider from an ActivityInboundInterceptor possible without ActivityInboundInterceptor needing to know anything about IServiceProvider.

Why?

I want to be able to use the scoped IServiceProvider instance used to construct the current Activity from an ActivityInboundInterceptor that is dealing with a cross-cutting concern that would benefit from access to the DI service provider.

For example, I have an ActivityInboundInterceptor, call it UserInfoInterceptor that interrogates the Activity to figure out what user work is being performed for and then makes that information available to other DI services that care about that context using a service registered with the IServiceProvider. I'd now like to add a new ActivityInboundInterceptor, call it LogDecoratorInterceptor, that decorates the log context with details about the user. This change would allow me to use the mechanism that UserInfoInterceptor provides via IServiceProvider instead of having to recompute the user or do other black magic.

Checklist

  1. Closes #363

  2. How was this tested:
    Not tested, just a proposal (though I do expect that it works)

  3. Any docs updates needed?
    I would think an example would be needed because even with this accessor, things are more convoluted than might be ideal. Consider:

During the build phase of the application, it would be necessary to create an instance of ServiceProviderAccessor and provide it to any IWorkerInterceptor that cares about accessing the scoped IServiceProvider instance later. The code would then need to register the ServiceProviderAccessor instance with the ServiceCollection being constructed as a Singleton so that ServiceProviderExtensions.CreateTemporalActivityDefinition could fetch the ServiceProviderAccessor instance later to register the scoped IServiceProvider instance.

To try to put that in code, here's a hypothetical Program.cs

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Temporalio.Extensions.Hosting;
using Temporalio.Worker.Interceptors;

namespace Temporalio.Issue363Demo;

internal class Program
{
    private static async Task Main(string[] args)
    {
        HostApplicationBuilder builder = Host.CreateApplicationBuilder(args);

        var temporalWorkerBuilder = builder.Services.AddHostedTemporalWorker(
            buildId: null, taskQueue: "TASK_QUEUE");

        ServiceProviderAccessor serviceProviderAccessor = new();
        MyInterceptor myInterceptor = new(serviceProviderAccessor);
        builder.Services.AddSingleton<IServiceProviderAccessor>(serviceProviderAccessor);

        temporalWorkerBuilder.ConfigureOptions(options =>
        {
            options.Interceptors = [myInterceptor];
        });

        var host = builder.Build();
        // ...
    }
}

public class MyInterceptor : IWorkerInterceptor
{
    private readonly IServiceProviderAccessor serviceProviderAccessor;

    public MyInterceptor(IServiceProviderAccessor serviceProviderAccessor)
    {
        this.serviceProviderAccessor = serviceProviderAccessor;
    }

    public ActivityInboundInterceptor InterceptActivity(
        ActivityInboundInterceptor next) =>
            new MyActivityInboundInterceptor(serviceProviderAccessor, next);
}

public class MyActivityInboundInterceptor : ActivityInboundInterceptor
{
    private readonly IServiceProviderAccessor serviceProviderAccessor;

    public MyActivityInboundInterceptor(IServiceProviderAccessor serviceProviderAccessor,
        ActivityInboundInterceptor next)
        : base(next)
    {
        this.serviceProviderAccessor = serviceProviderAccessor;
    }

    public override Task<object?> ExecuteActivityAsync(ExecuteActivityInput input)
    {
        var serviceProvider = serviceProviderAccessor.ServiceProvider;
        // Do something with serviceProviderAccessor!
        return Task.FromResult<object?>("yay!");
    }
}

@CLAassistant
Copy link

CLAassistant commented Nov 5, 2024

CLA assistant check
All committers have signed the CLA.

@@ -0,0 +1,16 @@
using System;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,42 @@
using System;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdg5 tdg5 force-pushed the tdg5/service-provider-accessor branch from 6b175cf to 886015d Compare November 5, 2024 18:36
@@ -68,6 +68,14 @@ public static ActivityDefinition CreateTemporalActivityDefinition(
#else
var scope = provider.CreateScope();
#endif
IServiceProviderAccessor? serviceProviderAccessor =
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inspiration taken from ASP.NET's DefaultHttpContextFactory.

/// </summary>
public class ServiceProviderAccessor : IServiceProviderAccessor
{
private static readonly AsyncLocal<ServiceProviderHolder> ServiceProviderCurrent = new();
Copy link
Author

@tdg5 tdg5 Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A System.Collections.Concurrent.ConcurrentDictionary that is keyed on the Activity's unique ID could be a simpler replacement for this and would work just as well since it would depend on the AsyncLocal holding the current Activity.

@tdg5 tdg5 force-pushed the tdg5/service-provider-accessor branch from 886015d to b657fe0 Compare November 6, 2024 02:58
@cretz
Copy link
Member

cretz commented Nov 7, 2024

Thanks! Hrmm, I am unsure we want to provide an async local form of what users can provide themselves. .NET expects, if you want access to the service provider, you inject it like anything else. Why not just accept service provider in the activity class constructor if you need it? Something like:

public class MyActivities
{
    private readonly IServiceProvider serviceProvider;
    private readonly IWhateverElse whateverElse;

    public MyActivities(IServiceProvider serviceProvider, IWhateverElse whateverElse)
    {
        this.serviceProvider = serviceProvider;
        this.whateverElse = whateverElse;
    }

    [Activity]
    public string DoMyActivity(string someParam)
    {
        // serviceProvider available here
        throw new NotImplementedException("TODO");
    }
} 

This way IServiceProvider is not special, it's just injected like anything else might be. If you as a user wanted to put some injected thing (like service provider) into an async local, you can.

@tdg5
Copy link
Author

tdg5 commented Nov 9, 2024

@cretz, thanks for the suggestion, but how do I that with an interceptor?

The issue I'm trying to find a solution for relates to handling a cross-cutting concern that would need to be handled in every Activity.

For example, consider adding support for OpenTelemetry in every Activity compared to adding support for OpenTelemetry via an interceptor that allows each Activity to be agnostic of OpenTelemetry and to instead focus on its actual job.

Support for OpenTelemetry avoids needing DI because diagnostic activities don't require a reference to the specific trace provider. But if that weren't the case, if supporting OpenTelemetry required the interceptor to have a reference to the specific trace provider and that trace provider were only accessible via the service provider, how would support for OpenTelemetry be handled other than by adding logic to every Activity to handle OpenTelemetry concerns via the injected IServiceProvider? It would no longer be a problem that could be solved with an interceptor.

Put another way, if there's some common, scoped service registered with the IServiceProvider, why should I have to add logic to every Activity in order to handle setup/teardown with that scoped service?

Put yet another way, if I were making a library for use with Temporal, how could I provide a library that depended on a scoped service from IServiceProvider and didn't need to be explicitly called in every Activity?

@cretz
Copy link
Member

cretz commented Nov 12, 2024

thanks for the suggestion, but how do I that with an interceptor?

An interceptor cannot control what an activity implementer chooses to dependency inject. This same concern occurs with anything to dependency inject not just service provider.

Support for OpenTelemetry avoids needing DI because diagnostic activities don't require a reference to the specific trace provider. But if that weren't the case, if supporting OpenTelemetry required the interceptor to have a reference to the specific trace provider and that trace provider were only accessible via the service provider, how would support for OpenTelemetry be handled other than by adding logic to every Activity to handle OpenTelemetry concerns via the injected IServiceProvider?

DI is optional in .NET and I believe the reason that OpenTelemetry and .NET diagnostic activities use a shared tracer provider and shared .NET diagnostic activity sources is because the per-DI-scope design is too limiting for general purpose use. I wonder if your use case could not rely on optional DI and scoping the same way OTel does not.

why should I have to add logic to every Activity in order to handle setup/teardown with that scoped service?

Because it is a specific need in your case to have implicit access to the DI container in a non-DI instantiated interceptor. One approach may be leveraging IServiceScopeFactory to set your async local. For example, maybe something like:

public class MyServiceScopeFactory : IServiceScopeFactory
{
    public static readonly AsyncLocal<IServiceProvider> CurrentServiceProvider = new();

    private readonly IServiceScopeFactory underlying;

    public MyServiceScopeFactory(IServiceScopeFactory underlying) => this.underlying = underlying;

    public IServiceScope CreateScope()
    {
        var scope = underlying.CreateScope();
        CurrentServiceProvider.Value = scope.ServiceProvider;
        return scope;
    }
}

And change host builder to have something like .ConfigureServices(services => services.AddSingleton<IServiceScopeFactory>(provider => new MyServiceScopeFactory(provider.GetRequiredService<IServiceScopeFactory>())) but I have not tested it.

The other option we'd entertain is a way to make the activity inbound interceptor be created in the current scope. My concern is exposing a static based on async local if we don't have to, but maybe we can have some ITemporalWorkerServiceOptionsBuilder extension like .AddScopedActivityInterceptor<ActivityInboundInterceptor> that we make sure somehow to instantiate with the service provider. I suspect we'd have to use ActivatorUtilities.CreateInstance to be able to provide the next interceptor. Maybe it would be powered internally by async local (i.e. an internal interceptor with static async local that is set the way this PR does, but remains internal). I would have to sit down and design this a bit.

@tdg5
Copy link
Author

tdg5 commented Nov 18, 2024

Hey @cretz , thanks for the thorough response.

I like your IServiceScopeFactory idea and have explored it, but it doesn't seem to be supported.

I'm out of time for this today, but will respond to your other comments later this week. ❤️

@cretz
Copy link
Member

cretz commented Nov 18, 2024

Interesting. So I am now starting to lean towards providing the service provider in the interceptor only. Meaning I wonder if we should keep this async local internal and have something like:

namespace Temporalio.Extensions.Hosting;

interface IWorkerInterceptorWithServiceProvider : IWorkerInterceptor
{
    ActivityInboundInterceptor InterceptActivity(ActivityInboundInterceptor nextInterceptor) =>
        InterceptActivity(WhateverInternalAsyncLocal.Value, nextInterceptor);

    ActivityInboundInterceptor InterceptActivity(IServiceProvider serviceProvider, ActivityInboundInterceptor nextInterceptor) =>
        nextInterceptor;
}

My main concern is I am not wanting people to generally use an async local to access the scope or service provider inside the activity if we can help it. I agree that constructor injection doesn't work with interceptors, so maybe this can work around it this way (still using async local internally though).

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 this pull request may close these issues.

[Feature Request] Make scoped IServiceProvider available to ActivityInboundInterceptor
3 participants