umbraco / Umbraco.Forms.Issues

Public issue tracker for Umbraco Forms
29 stars 0 forks source link

WorkflowService kills performance of Umbraco Cloud hosted site #1283

Closed Alkaersig closed 3 days ago

Alkaersig commented 3 weeks ago

We launched a new Customer on Umbraco Cloud yesterday, and the site performed extremly poor after launch.

After alot of testing we boiled it down to pages having forms. After further testing we discovered that using the Umbraco Forms WorkflowService in the Cloud site had severe performance-issues. We used this service previously in older Umbraco projects (for the same purpose) in self-hosted environments and in local environments without any issues.

As part of a Consent Workflow we have, we use the service to test whether a given form has this workflow. On the site we launched yesterday this resulted in loading times raging from 3 seconds all the way up to 30 seconds in rare cases (in Cloud). After removing the use of the service (as a temporary fix hopefully) everything is running smoothly now.

I have not had time to dig into the implementation and nothing is to be found wihtin the Forms Documentation, but i guess it might be hitting the database hard.

Reproduction

Forms Version 13.2.1

We are making a simple call at render time to get the worksflows of a given form: var workflows = workflowService.Get(form);

Cloning the site and running it locally does result in a slighty increased loading-time (we talking 100ms-200ms) but running the same site in Cloud results in greatly increased loading-times.


This item has been added to our backlog AB#43346

ronaldbarendse commented 2 weeks ago

Hi Christian, thanks for reaching out to us about this! This level of performance/loading times indeed isn't acceptable, although it seems like it's caused by the custom implementation.

The IWorkflowService does direct database queries (like you already suspected), so not caching the returned result and/or calling it multiple times (e.g. for each rendered field) would indeed cause greatly increased loading times. This will be amplified on Cloud, as the database is not running locally and you'll likely have concurrent requests all hitting the database at the same time...

Can you elaborate a bit more on the checks you're doing? What is the Consent Workflow and how is the rendered form affected based on the result of the check? What about other factors, like the amount of forms and workflows, used Cloud plan/dedicated resources, etc.? I'll make sure to look into obvious improvements we can do on our side in the meantime 👍🏻

Alkaersig commented 2 weeks ago

Hello @ronaldbarendse

Thanks for looking into this.

The way we are using it is that we fetch the workflows of a given form at render time. Then we check the list of worksflow to see if our Consent workflow is present. If the workflow is present we render some extra fields that allows the user to consent (and sign up for email marketing etc.).

When the form is submitted the workflow will pick up the Consent and submit it to the external servicethat handles the consents.

So it is a single lookup for each form (most pages have just one form and it resulted in the very long loading times).

The customer is currently running on a starter, with memory usage around 50% and CPU time below 2 seconds most of the time, so resources should be fine.

ronaldbarendse commented 2 weeks ago

Thanks for the additional info!

I'd generally recommend against doing lookups on each request and instead use service notifications to set some state when the form/workflow is saved. In this case, you'd need to handle the WorkflowSavedNotification, check the workflow type and if it's the Consent workflow, store some state on the related form (like adding a fieldset, which automatically renders the extra fields). You'd need to do the opposite when the workflow saved with Active disabled and when it's deleted (by handling WorkflowDeletedNotification).

While looking into this, I did notice we're creating Umbraco scopes using RepositoryCacheMode.None in multiple places within Forms, which bypasses the default repository cache and always queries the database. I'll have to do some digging to figure out why this is done, stay tuned...

Alkaersig commented 2 weeks ago

No problem @ronaldbarendse

As stated this never led to any performance issues in previous setups, so it has not really been a concern (and i was kinda expecting the service using a cache layer since forms is using the service itself when working in the backoffice and caching is kinda well handled in Umbraco in general).

But we will look into an alternative implementation. It does not sound like a bad idea, we will probabl create a state collection that we can query and/or read to determine whether the workflow exists on a given form and keep it up to date with notifications (just as having a cache).

But thanks for looking into it, from your findings s far it sounds like things in forms could be handled better with some cache anyway.

ronaldbarendse commented 2 weeks ago

I did find out why we're currently not caching entities in Forms: the submitted values are stored in the form entity, workflow settings (as the values can contain record/request placeholders), etc. If these entities are cached, the same instance will be shared between requests, resulting in leaking submitted values/request specific data, which we definitely don't want 😇

The performance difference with older Umbraco projects running in self-hosted environments might be due to:

AndyButland commented 1 week ago

Just to comment here too - it could be the issue here is more related to the deserialization of the workflow details rather than the actual database query itself. I recall we had some issues related to this when retrieving forms, data sources and prevalue sources for the backoffice - which is why we introduced "slim" versions of these entities that you can retrieve via e.g. FormService.GetSlim().

We don't have the equivalents here for workflows.

Given you are only checking for the existence of a particular workflow, if these existed they could help here as we don't need the full definition of each workflow.

Alkaersig commented 1 week ago

@ronaldbarendse the customer is now running on a standard plan, i dont expect it to make a huge difference, but i will test it anyway to see how much the plan affects the performance of the service (out of curiosity).

That makes perfect sense @AndyButland, since all we really need here is a list of workflows (ids/keys/names) that exist on a given form and not the actually workflow data.

AndyButland commented 3 days ago

We will make available the ability to retrieve "slim" versions of the workflows via the services in the next releases.

The following shows how they could be used (this example is in a view, but of course the services can be used in controller or other C# service code as well):

@using Umbraco.Forms.Core.Services

@{
    @inject IFormService FormService;
    @inject IWorkflowService WorkflowService;

    if (Model.Form.HasValue)
    {
        var form = FormService.Get(Model.Form.Value);
        var workflows = WorkflowService.GetSlim(form);
        <ul>
            @foreach (var workflow in workflows)
            {
                <li>@workflow.Name (@workflow.Id)</li>
            }
        </ul>

        workflows = WorkflowService.GetSlim(Model.Form.Value);
        <ul>
            @foreach (var workflow in workflows)
            {
                <li>@workflow.Name (@workflow.Id)</li>
            }
        </ul>
    }
} 
Alkaersig commented 3 days ago

Looks good @AndyButland, thanks for coming up with a good solution, H5YR