elsa-workflows / elsa-core

A .NET workflows library
https://v3.elsaworkflows.io/
MIT License
6.26k stars 1.15k forks source link

[ENH] Enhance logging by adding logger state to activity and workflow pipelines #5984

Open bobhauser opened 4 days ago

bobhauser commented 4 days ago

Enhancement Request

Enhancement Overview

When viewing application logs, it is often useful to see contextual properties to help understand the logs. For instance, when viewing a log message it might be useful to know the workflow instance Id of the workflow being executed, or the activity type of the current activity. Adding this sort of context is usually accomplished through the use of the ILogger.BeginScope method. The desired format of the logger state is often very specific to the application. For instance, we use structured logging (serilog/datadog), and we might like to see the logger state nested like

workflow 
    / name
    / instance_id
    / activity
         / id
         / type

but of course everyone would not like it like that... so the logger state generation should be injectable.

Proposed Enhancement

I propose that new middleware be added to Elsa, both for the workflow pipeline and activity pipeline. The new middleware would make use of an injectable ILoggerStateGenerator (name TBD) which would return the logger state for the given context object. The default implementation may just add WorkflowInstanceId / ActivityId properties, where as a custom implementation could be injected if requried.

Note: WorkflowInstanceId logger state is currently added directly by WorkflowRunner.RunAsync - this should be removed in preference to the proposed middleware if we end up going with this proposal.

Alternative Solutions

Although I believe new pipeline middleware would be the cleanest approach, it would also be possible to have the code which triggers the pipeline execution set the logger state around the pipeline execution. As I see it, this would be in WorkflowRunner.RunAsync for workflow scope (in place of there it already adds the WorkflowInstanceId logger state) and ActivityInvoker.InvokeAsync for activity scope.

Additional Context

We currently have this implemented using custom middleware and would be happy to contribute this to Elsa. Or, if the alternative solution is preferred, we'd be happy to do this work too.

bobhauser commented 4 days ago

On further reflection, I think that the alternative solution mentioned above would be the best approach as it would apply even in situations where the default workflow and/or activity pipelines have been overwritten.

Also, a scope is already defined in WorkflowRunner.RunAsync - moving the scope creation into a pipeline middleware would mean that the scope would no longer be applied around notifications send using notificationSender.