Add IProcessEnd interface

[danielskovli]

Description

As discussed briefly within the team, though not necessarily agreed upon, this is a proposal to add an IProcessEnd interface to the process engine.

Following the established convention, this would involve creating and registering a WhateverEventHandler. In this particular case, based on the names of the other handlers, the name would probably be EndProcessEventHandler or something similar. However, we already have an EndEventEventHandler, which also gets invoked at the end of the process.

So to avoid further confusing the issue, I simply added this new feature as an IEnumerable<IProcessEnd> injection straight in ProcessEngine.

Motivation

A service owner wishes to place a hook in their app code, to be executed after the instance process has come to an end. In the case at hand, their wish is to place a call to /complete to flag the instance as "read". This is typically done after the instance has completed its processing and has arrived at endEvent.

We currently have IAppEvents.OnEndAppEvent which gets invoked at the end of a process, but it's not really intended for public implementation and the default handler contains logic which we want to keep. Additionally, the invocation of OnEndAppEvent doesn't actually happen right at the end of the process:

1. Call to ProcessEventHandlerDelegator.HandleEvents which eventually calls OnEndAppEvent
2. Call to ProcessEventDispatcher.DispatchToStorage which dispatches the final instance update to Storage
3. Events are dispatched

I've actually lost track of whether it matters or not, that the OnEndAppEvent fires before the last call to Stoage. But regardless of that, the overall vibe of the uphill battle I've had here leads me to think that implementing an actual IProcessEnd interface is a reasonable solution.

Verification

• Your code builds clean without any errors or warnings
• Manual testing done (required)
• Relevant automated test added (if you find this hard, leave it and we'll help out)
• All tests run green

Documentation

• User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

[martinothamar]

I think this makes sense to have long term as well 👍
Open questions

• IProcessEnd vs IInstanceEnd? I like the process one but I guess we could say that both the instance and the process is done here, so thought I'd bring it up
• For symmetry we could consider IProcessStart or IInstanceStart I guess, but we already have hooks for the start variant of this.

It would have been useful to have a concrete list of events/hooks we want to have in the future process engine (v9). We should probably do that soon...

Does backport-ignore make more sense here since this is a new feature?

[danielskovli]

@martinothamar

IProcessEnd vs IInstanceEnd? I like the process one but I guess we could say that both the instance and the process is done here, so thought I'd bring it up

Good question! I don't have any strong opinions on this, I only went with process because it was an established term. Instance would make it clearer, no doubt about that. I'd be happy to take direction on this.

For symmetry we could consider IProcessStart or IInstanceStart I guess, but we already have hooks for the start variant of this.

Definitely! I only skipped this part because I don't need it for the issue I'm working on right now, so I figured it could be added later if we wanted it. I also suspect that IInstantiationProcessor covers most of the usecases I can think of for an IInstanceStart hook, so I'm also not entirely sure about the user story here.

Does backport-ignore make more sense here since this is a new feature?

I did this on purpose, because I am hoping to get this released very soon (assuming everyone's cool with that). But other than that, I completely agree that this doesn't really fit the usual "backport" mindset. Happy to take some advice on this too 🙂

[ivarne]

I will likely vote for Process. I don't think it makes much sense to hide the fact that we have a process backing the instance.

1. Should we include IInstanceDataMutator?
2. Telemetry around execution of the hook
3. Can/should we support parallel execution of multiple hooks? It kind of makes sense to be able to run multiple reporting jobs simultaneously.

[danielskovli]

@ivarne

Should we include IInstanceDataMutator?

This isn't available in ProcessEngine, so I'm not sure exactly how that would flow out to these invocations. Other than that, sure!

Telemetry around execution of the hook

We haven't done anything with telemetry for other user hooks, as far as I can see. So I didn't feel it was necessary here either, but perhaps this could be a bigger lift at some point?

Can/should we support parallel execution of multiple hooks? It kind of makes sense to be able to run multiple reporting jobs simultaneously.

My initial thought is that this possibly breaks with user expectations, since we're invoking all other user code sequentially?

[danielskovli]

/publish

[github-actions]

PR release:

• Altinn.App.Api 8.6.0-pr.557.iprocessend-interface.fb8e63ca
• Altinn.App.Core 8.6.0-pr.557.iprocessend-interface.fb8e63ca

⚙️ Building...
✅ Done!

[ivarne]

We haven't done anything with telemetry for other user hooks, as far as I can see. So I didn't feel it was necessary here either, but perhaps this could be a bigger lift at some point?

We do in dataProcessing and validation

app-lib-dotnet/src/Altinn.App.Api/Helpers/Patch/InternalPatchService.cs

Lines 273 to 289 in b347d72

	}
	using var processWriteActivity = _telemetry?.StartDataProcessWriteActivity(dataProcessor);
	try
	{
	await dataProcessor.ProcessDataWrite(
	dataMutator.Instance,
	change.DataElementIdentifier.Guid,
	change.CurrentFormData,
	change.PreviousFormData,
	language
	);
	}
	catch (Exception e)
	{
	processWriteActivity?.Errored(e);
	throw;
	}

Validation does it in parallel, but IProcessEnd would need to be opt-in for parallel execution, so we can add that later if the new process engine wants to support it in some way.

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud