heap allocation optimization for JsonSerializerOptions

[sungam3r]

Relates to #385

@CarlosLanderas It seems to me that it will be much better and the advantage in comparison with the old sync version will increase more.

[CarlosLanderas]

@sungam3r, thanks for trying to improve the Response writer even more. But the signature you need to provide to the ResponseWriter is defined by Microsoft as:

public Func<HttpContext, HealthReport, Task> ResponseWriter { get; set; }

And we do not manually invoke the method:

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/master/samples/HealthChecks.UI.Branding/Startup.cs#L61

The only way I see something like this being done would be wrapping the new public function:

 var options = new JsonSerializerOptions(); 
//whatever config in JsonSerializer..

.UseEndpoints(config =>
   {
     config.MapHealthChecks("/health-random", new HealthCheckOptions
      {
       Predicate = r => r.Tags.Contains("random"),
       ResponseWriter = async(context, report) =>
       {
           //Wrap the new implementation that caches the options
           await UIResponseWriter.WriteHealthCheckUIResponse(context, report, options);
        }
 });

But the problem is we can't do this as there is danger in allowing external JsonOptions configuration, as you can see there are converters that are a must for retrocompability and that are being used by the frontend spa UI:

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/master/src/HealthChecks.UI.Client/UIResponseWriter.cs#L35

[sungam3r]

So I did not change the signature.

[CarlosLanderas]

 public static async Task WriteHealthCheckUIResponse(HttpContext httpContext, HealthReport report, Action<JsonSerializerOptions> jsonConfigurator)

Should be a private method then?

[sungam3r]

No. Why?

[sungam3r]

I just cached the settings object, which is used constantly if no configuration delegate is set.

[CarlosLanderas]

To be honest, I do not know why that second method is public when the ResponseWriter is automatically invoked by Microsoft Healthchecks and that signature is not compatible, and it also does not make sense to use that Action because the converters can't be externally configured as they should be fixed. I know that was already there...

For me, the best implementation would be this:

 public static class UIResponseWriter
    {
        private static byte[] emptyResponse = new byte[] { (byte)'{', (byte)'}' };
        private static Lazy<JsonSerializerOptions> options = new Lazy<JsonSerializerOptions>(() => CreateOptions());

        const string DEFAULT_CONTENT_TYPE = "application/json";

        public static async Task WriteHealthCheckUIResponse(HttpContext httpContext, HealthReport report)
        {
            if (report != null)
            {
                httpContext.Response.ContentType = DEFAULT_CONTENT_TYPE;

                var uiReport = UIHealthReport
                    .CreateFrom(report);

                using var responseStream = new MemoryStream();

                await JsonSerializer.SerializeAsync(responseStream, uiReport, options.Value);
                await httpContext.Response.BodyWriter.WriteAsync(responseStream.ToArray());
            }
            else
            {
                await httpContext.Response.BodyWriter.WriteAsync(emptyResponse);
            }
        }

        private static JsonSerializerOptions CreateOptions()
        {
            var options = new JsonSerializerOptions()
            {
                AllowTrailingCommas = true,
                PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
                IgnoreNullValues = true,
            };

            options.Converters.Add(new JsonStringEnumConverter());

            //for compatibility with older UI versions ( <3.0 ) we arrange
            //timespan serialization as s
            options.Converters.Add(new TimeSpanConverter());

            return options;
        }
    }

[sungam3r]

As you wish.

[sungam3r]

But wait...

 .UseHealthChecks("/healthz", new HealthCheckOptions
                {
                    Predicate = _ => true,
                    ResponseWriter = MyDelegate
                })
...
private static Task MyDelegate(HttpContext httpContext, HealthReport report)
        {
            return UIResponseWriter.WriteHealthCheckUIResponse(httpContext, report, opt => opt.Converters.Add(new MyConverter())); // whatever
        }

[CarlosLanderas]

PD: Updated above sample.

We should not let users configure the UI Response writer json options as it can break the frontend UI Spa. Is a response designed for the UI, that's why is called UI.Client.

That's why I told that already existing public method with an options configuration is not being used and makes no sense.

[sungam3r]

I do not really understand. So to change just one serializer setting I need to copy-paste all UIResponseWriter default implementation? Do you mean that?

[CarlosLanderas]

No. I mean, no JsonSerializerOptions should be configurable from outside (public API). It should follow a fixed non configurable json format to work with the react SPA UI. Allowing users to configure the Json Settings might break the render in the browser

So that method using an Action that already was there, makes no sense and the better thing is removing it in this PR.

The usage should continue being

ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse

As no json configuration is needed and we can use lazy static initialization for internal fixed json options as shown here:

#386 (comment)

[sungam3r]

Now I understand your point.

Regarding this:

Allowing users to configure the Json Settings might break the render in the browser

and that:

The usage should continue being
ResponseWriter = UIResponseWriter.WriteHealthCheckUIResponse

Honestly, you already allow the user to set the entire delegate, so they can always break something for UI anyway. So what's the difference?

[CarlosLanderas]

The user is free to use whatever response writer they want of course, but if they want to use our UI, they must use our UI client method as delegate and that is shown in the docs. The react spa knows how to render that response but not custom ones for obvious reasons.

That's why this method is sealed from external configurations

[CarlosLanderas]

For example, the default Microsoft response writer just writes "Healthy" or "Unhealthy" so we rolled out our own UI version that is coupled with our UI front-end so the user does not have to implement by himself

[sungam3r]

OK, the general meaning is clear, the rest is not so important. The main thing is that code has become better.

[CarlosLanderas]

Thanks for the contribution @sungam3r

[cjbush]

I contributed the original method that allowed you to configure the JSON serializer. The reason it was added (and was being used) was to allow you to format your healthcheck names with something other than the default formatting (e.g. if I want my healthcheck name to be "My Database Connection" instead of "my Database Connection"). Removing the method is a breaking change and should cause a major version bump. I just installed the latest package and now all of my configurations are broken, so I'll have to roll back.

[sungam3r]

Removing the method is a breaking change

It was obvious enough, but I did not insist and agreed with @CarlosLanderas to remove this method.

[unaizorrilla]

This is a breaking change and probably we need to restore old way and deprecate current package.

@CarlosLanderas can you revert this?

[CarlosLanderas]

Sure, we will stay with the initial async version with less allocations I merged and restore the previous signatures to avoid the breaking change

We will change the behaviour in the future to allow the users transforming the output but not configuring the serializer settings directly as it might break the UI.

@cjbush I'll publish the new version tonight, sorry for the inconvenience

[sungam3r]

Will I need to redo my initial changes?

[CarlosLanderas]

@sungam3r propose them in a new PR if you want 👍. I remember the initial version with settings caching and old signatures. That should work.

[cjbush]

@CarlosLanderas It's ok. You make a good point about breaking things by configuring the serializer too much. If you want, I can submit another patch that deprecates the current method that allows that and adds another method that takes a settings object with whatever settings you think are appropriate. Something like:

public class HealthcheckDisplayOptions
{
    public bool UseCamelCase { get; set; } //or an enum or something
}

public static async Task WriteHealthCheckUIResponse(HttpContext httpContext, HealthReport report, Action<HealthcheckDisplayOptions> config)
{ 
/* Handles configuring the serializer based on the options passed in */ 
}

[CarlosLanderas]

Sounds good 👍. That DisplayOptions object is good to add new features that users might need and configure them internally without having the chance of accidentally clear the necessary converters or mandatory configuration for the UI

[sungam3r]

So will there be a rollback or not (another patch)?