Ability to monitor Blazor Server circuit activity

[MackinnonBuck]

Ability to monitor Blazor Server circuit activity

Adds the ability to monitor circuit events such as unhandled exceptions and JS interop calls.

Description

This PR adds functionality to the existing CircuitHandler to also support non-lifecycle circuit events.

Opening initially as a draft PR to gather feedback. I'll add new tests after we validate the overall direction.

Fixes #30287

[MackinnonBuck]

The additional handlers I've added so far are:

• Unhandled exceptions
• Starting an invocation to .NET from JS
• Ending an invocation to JS from .NET

The start/end disparity for the interop method handlers was to keep all circuit handler invocation logic in CircuitHost. This was sort of an arbitrary decision, so I'm definitely happy to reconsider it.

Some other potential CircuitHandler events:

• A render completes
• The page's location changes
• Anything related to transferring streams/arrays between .NET and JS

[SteveSandersonMS]

Before we close on this, can we step back and work through the design justification? I know that #30287 says "callbacks when an event, JS interop call, or other activity" which in turn is copied from #27885, but we would be wise to spell out the customer scenarios this addresses. I don't think we went through any structured design process to get here (and apologies for that - we just filed vague issues and then ran with it).

The one scenario I know for sure is "detecting idleness", however that could be covered by a much more general OnCircuitActivity method which would also have the benefit of not providing too much granularity of detail about internal implementation details. There is some risk that by exposing, say, OnEndInvokeJSFromDotNetAsync and OnInvokeDotNetFromJSAsync, we'll get into situations where modifying the implementation of internal .NET-JS communications for random features could end up breaking people's apps because they were relying on these callbacks occurring at exact points in the flow. Basically, the more detail we put into these callbacks, the more we are creating opportunities for future breakage or restrictions on changes we'd be comfortable making.

Let's make sure we don't try to come up with scenarios that happen to fit this design in a post-hoc way, but rather consider if there are viable options to minimize the API surface to keep options open.

[javiercn]

My original view on this scenario has always been to extend the current support that we have for circuithandlers to also support other interactions.

The goal for me is that we have some sort of filter/middleware where people can perform centralized actions like:

1. Detecting idleness to make decisions on the circuit lifetime.
2. Setting up circuit state (async locals) for things like HttpClientFactory Exception 'GetAuthenticationStateAsync was called before SetAuthenticationState.' thrown when calling AuthenticationStateProvider.GetAuthenticationStateAsync() method #28684
3. Setting up circuit state for non-conforming containers Intercepting Blazor page events. #30115

I am fine if we add a single method "RegisterCircuitActivityEvent" to CircuitHandler that gets called to register a callback with the shape Func<EnumEventType, Circuit, Func<Task>, Task> with the idea that people can write code like this:

async (evt, circuit, handler) => { 
  Class.AsyncLocal = value;
  await handler();
  Class.AsyncLocal = null;
}

Or in the future tackling other things like terminating inactive circuits.

I don't want us to expose any of the internals of the hub method parameters or anything like that.

[SteveSandersonMS]

In your scenario list, is item 3 a special case of item 2? Or is some distinct API required for item 3?

I'm speculating that what's common to all cases is the idea of "inbound activity". None of the cases require differentiating what sort of inbound activity it is (and it would be ideal not to put that on the API), but it is meaningful to talk about the activity being inbound because people would not want to set up async local state around outbound activity.

[javiercn]

In your scenario list, is item 3 a special case of item 2? Or is some distinct API required for item 3?

It's fair to say that they are the same, just adding concrete examples of specific problems.

• Another one is being able to use things like AuthenticationStateProvider or JS interop when you are using OwnedComponentBase. You can think of users setting up an AsyncLocal for the IJSRuntime or the AuthenticationStateProvider in their code to make these scenarios work.

[SteveSandersonMS]

OK great, that helps to clarify things a lot. So can we nail down a definite API proposal?

The simplest reasonable starting point, which is almost exactly what @MackinnonBuck has so far:

namespace Microsoft.AspNetCore.Components.Server.Circuits;

public abstract class CircuitHandler
{
+    public virtual Task OnInboundActivityAsync(Circuit circuit, CancellationToken cancellationToken);
}

With the above known scenarios, can we get away with it being this simple?

Questions:

• Should it really be RegisterCircuitInboundActivityEvent like suggested above? Does that mean people also need to be able to unregister? As a drawback it's different from the other calls on CircuitHandler, making the API feel quite inconsitent.
• Do people strictly need to run code after the activity completes for the known scenarios? If so that complicates the implementation a bit and might force us to allocate a bunch more tasks/completion sources to process them hierarchically. And it's also different from the other calls on CircuitHandler.

[javiercn]

• Should it really be RegisterCircuitInboundActivityEvent like suggested above? Does that mean people also need to be able to unregister? As a drawback it's different from the other calls on CircuitHandler, making the API feel quite inconsitent.

I am fine with the simpler API, but the idea was that if you don't register anything, we skip it alltogether. Maybe we can use an additional interface to signal that.

• Do people strictly need to run code after the activity completes for the known scenarios? If so that complicates the implementation a bit and might force us to allocate a bunch more tasks/completion sources to process them hierarchically. And it's also different from the other calls on CircuitHandler.

It needs to wrap the call we make, otherwise AsyncLocals won't work. So it needs to be something like:

Dispatcher.InvokeAsync(() => handler.Invoke(circuit, () => dispatchEvent(...))

If we don't want to "close" over variables, we can pass in a handler(CircuitInvocationContext),CircuitInvocationContext) where CircuitInvocationContext is a struct we populate with whatever we need and that it is opaque to the caller.

[MackinnonBuck]

Thanks, @javiercn and @SteveSandersonMS! The scenarios are clearer to me now.

I've updated my prototype to introduce just a single new method:

namespace Microsoft.AspNetCore.Components.Server.Circuits;

public abstract class CircuitHandler
{
+    public virtual Task HandleInboundActivityAsync(Circuit circuit, Func<Task> next);
}

I named it HandleInboundActivityAsync instead of OnInboundActivityAsync because unlike the other handler methods, this one has a responsibility: it must invoke the provided next function. I think using the "On" prefix implies that the method is purely an observer, which isn't the case; it's an active participant. The downside is that it's inconsistent with the other CircuitHandler methods.

This leads to another question - does this method even belong in CircuitHandler? I would probably say yes, because:

• While the new method can be used as a middleware to do things set/unset async locals, it doesn't have to be used that way. It's totally possible you want to do something purely monitoring-related like idleness detection, and that seems to align with the goals of the existing circuit handler methods. It might feel weird to go through an additional mechanism to enable that scenario.
• The existing CircuitHandler is a convenient place for this to go. The Order property already solves the problem of deciding the order of the pipeline. Although, maybe an alternative could be some other configuration mechanism akin to ASP.NET Core middleware configuration?

Some other questions I have:

• Should we provide any information about the specific event being dispatched?
  • An enum like CircuitEventType? How granular do we get with its values?
    • If we had one value for each CircuitHost method representing an inbound event, we'd be tying public API to our internal implementation.
    • We could have one value for each category of event:
      • Circuit lifetime
      • Rendering
      • JS<->.NET interop
      • Navigation
  • A CircuitEventContext read-only struct to replace the existing circuit argument. This contains:
    • The circuit in question
    • Internal properties useful to us
    • Information we add in future improvements (event type, etc.)
• Do we want to consider circuit lifecycle events to be "inbound" activity, even though these are already represented by existing CircuitHandler methods?
  • These seem like a separate category of activity to me. I would imagine lifecycle methods having a different meaning than other inbound activity when it comes to idleness detection, for example.

Thoughts?

[SteveSandersonMS]

I named it HandleInboundActivityAsync instead of OnInboundActivityAsync because unlike the other handler methods, this one has a responsibility: it must invoke the provided next function. I think using the "On" prefix implies that the method is purely an observer, which isn't the case; it's an active participant.

Great point. Totally agreed!

This leads to another question - does this method even belong in CircuitHandler? I would probably say yes

Also agreed. I don't see any real drawback to it being here - it's the most natural place in the existing API structure.

Should we provide any information about the specific event being dispatched?

Probably not at this point. Our known scenarios don't include that requirement, and the more detail we expose, the more our internal implementation choices become limited.

I do concur with your suggestion about A CircuitEventContext read-only struct to replace the existing circuit argument. I think that's also roughly what Javier was suggesting. Example:

public abstract class CircuitHandler
{
+    public virtual Task HandleInboundActivityAsync(CircuitInboundActivityContext context, Func<Task> next);
}

+// It's important to have at least one public field otherwise adding any other fields in the future would be breaking
+// See "Adding an instance field to a struct that has no nonpublic fields" in https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules
+public readonly struct CircuitInboundActivityContext
+{
+    public Circuit Circuit { get; }
+}

... and then we're free to add other info to CircuitInboundActivityContext in the future if we want, without any breaking API change.

Do we want to consider circuit lifecycle events to be "inbound" activity ... These seem like a separate category of activity to me

Also agreed that it doesn't seem like we'd want to define all activity as being "inbound". We can document the kinds of things that trigger this callback.

[javiercn]

It will be interesting that we update the docs for HttpClientFactory with this approach, as well as offer other examples, like making values available to OwningComponentBase and other components.

[SteveSandersonMS]

The following is nitpicky so feel free to ignore it, especially if you think it makes the code unpleasant!

It may be possible to avoid the extra allocation caused by capturing these locals in the closure by redefining HandleInboundActivityAsync as HandleInboundActivityAsync<T>(T data, Func<T, Task> callback), then passing in the otherwise-captured values as the T data parameter (for example, the caller could pass a tuple containing whatever combination of values they want). The lambda can then be defined as static.

Not sure if that's justified or would make the code hard to read.

[MackinnonBuck]

I think in order to make this work, we would have to include data in the CircuitInboundActivityContext class so it can be passed all the way through to the last handler (otherwise a capture would have to happen somewhere else). But then we would probably need to make CircuitInboundActivityContext generic in order to avoid boxing the data parameter. And then we would need to introduce a non-generic base type for CircuitInboundActivityContext to avoid also making IHandleCircuitActivity.HandleInboundActivityAsync generic.

Or maybe there's a way simpler way of doing this that I'm missing, but if it really does require that we do all this ^ then I think it might be best to leave as is. Is that okay?

[javiercn]

I am ok if we leave it as is for now, and file an issue to revisit. If in the case that nobody registers an IHandleCircuitActivity implementation we don't allocate, I think it is ok to leave it like this for now.

[SteveSandersonMS]

Nice!