Skip to content

[API Proposal] Ability to monitor circuit activity #47325

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

Closed
MackinnonBuck opened this issue Mar 20, 2023 · 2 comments · Fixed by #47328
Closed

[API Proposal] Ability to monitor circuit activity #47325

MackinnonBuck opened this issue Mar 20, 2023 · 2 comments · Fixed by #47328
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components

Comments

@MackinnonBuck
Copy link
Member

Background and Motivation

Note: This was implemented in #46968

In Blazor Server apps, it's sometimes desirable to access Blazor services from a non-Blazor DI scope. Examples:

We currently document a solution here. It involves wrapping each asynchronous entry point into Blazor component code and capturing the Blazor context's IServiceProvider in an AsyncLocal. Blazor services can then be referenced via the async local if the async call stack originates from Blazor code.

However, this approach has its limitations:

  • It requires copying and pasting parts of Blazor's ComponentBase implementation
  • It only works for components that extend a custom ComponentBase derivation implementing the wrapper methods

Separately, another common ask is to detect circuit idleness in support of custom circuit eviction logic.

Both these problems can be solved by allowing customers to inject a sort of "middleware" that runs for all incoming circuit activity that causes application code to run.

Proposed API

namespace Microsoft.AspNetCore.Server.Circuits;

+public sealed class CircuitInboundActivityContext
+{
+    public Circuit Circuit { get; }
+}

+public interface IHandleCircuitActivity
+{
+    Task HandleInboundActivityAsync(CircuitInboundActivityContext context, Func<CircuitInboundActivityContext, Task> next);
+}

Usage Examples

To utilize the feature, create a class that extends CircuitHandler (required) and implements IHandleCircuitActivity. Then, add the class as a service to the DI container.

Here's an example showing how to make Blazor services accessible to non-Blazor scopes:

BlazorServiceCircuitHandler.cs

public class BlazorServiceCircuitHandler : CircuitHandler, IHandleCircuitActivity
{
    private readonly IServiceProvider _services;
    private readonly BlazorServiceAccessor _bazorServiceAccessor;

    public BlazorServiceCircuitHandler(IServiceProvider services, BlazorServiceAccessor blazorServiceAccessor)
    {
        _services = services;
        _bazorServiceAccessor = blazorServiceAccessor;
    }

    public async Task HandleInboundActivityAsync(CircuitInboundActivityContext context, Func<CircuitInboundActivityContext, Task> next)
    {
        _bazorServiceAccessor.Services = _services;
        await next(context);
        _bazorServiceAccessor.Services = null;
    }
}

BlazorServiceAccessor.cs

public class BlazorServiceAccessor
{
    private static readonly AsyncLocal<IServiceProvider> s_currentServices = new();

    public IServiceProvider? Services
    {
        get => s_currentServices.Value;
        set => s_currentServices.Value = value;
    }
}

Program.cs

// ...
services.AddScoped<CircuitHandler, BlazorServiceCircuitHandler>();
services.AddScoped<BlazorServiceAccessor>();
// ...

Alternative Designs

We considered adding a new method to the CircuitHandler class, but that introduces unnecessary overhead when the HandleInboundActivityAsync() is not overridden.

We also thought about including information about what "type" of event is being handled, but we decided against it because it ties our internal implementation to the public API, restricting us from making drastic changes to the area.

Risks

No known risks. This feature is additive and shouldn't change the behavior of existing apps.

@MackinnonBuck MackinnonBuck added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 20, 2023
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Mar 20, 2023
@ghost
Copy link

ghost commented Mar 20, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented Mar 20, 2023

API Review Notes:

  • How long does a DI scope last for server-side blazor? The lifetime of the circuit.
  • Is encouraging AsyncLocal usage like IHttpContextAccessor really a good idea? AsyncLocal can be confusing.
  • Why not make HandleInboundActivityAsync a new virtual on CircuitHandler? So we don't have a bunch of wrapping next calls that do nothing when it's not customized.
    • It's not as discoverable on an interface.
    • Could we make it virtual and return Func<CircuitInboundActivityContext, Task> instead of Task?
      • This would avoid redundant wrapping when not overridden.
      • Might be less convenient when you do want to override it.
  • Do we want to expose SignalR's ConnectionContext to store state? It would be hard to flow this everywhere.
  • Is this even useful if you don't set an AsyncLocal? Maybe for logging. Not much else.
    • Could we just expose an IBlazorServiceAccessor and not the HandleInboundActivity?
      • HandleInboundActivity could be useful if we add more API to Circuit like the ability to close it.
  • Is CircuitInboundActivityContext testable? Can we just use Circuit directly? There's some internal state outside of the circuit we store in the context.
    • It's not really possible mock a Circuit today anyway. If one day that's a thing, we can add a constructor.
  • Do we need a CancellationToken? We wouldn't use it right now.
    • Could we add it to CircuitInboundActivityContext later if we need it? Possibly. It's not super standard, but it's what we do for HttpContext and middleware.

API Approved!

namespace Microsoft.AspNetCore.Components.Server.Circuits;

+public sealed class CircuitInboundActivityContext
+{
+    public Circuit Circuit { get; }
+}

public abstract CircuitHandler
{
+    public virtual Func<CircuitInboundActivityContext, Task> CreateInboundActivityHandler(Func<CircuitInboundActivityContext, Task> next) => next;
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Mar 20, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants