Camunda Spring Boot Engine Events - Decide if they are really skipable

p-wunderlich commented 1 year ago

User Story (Required on creation)

As a software developer, I want to implement unskippable Spring Event handlers in the Spring Boot starter so that I can implement critical business logic that end users should not be inadvertently able to skip

Functional Requirements (Required before implementation)

There is a new configuration property to mark the Spring Eventing as not skippable
When activated, the Spring Event handlers are registered as built-in listeners so that API calls that set the skipCustomListeners flag do not skip them.
Default behavior remains the same to keep backwards compatibility

Technical Requirements (Required before implementation)

Limitations of Scope

Hints

Current behaviour is, that when a user modifies a process in camunda cockpit, there is the checkbox where you can choose if the custom events should be skipped. Sadly this is per default true, so in this case, camunda spring boot engine events will not be fired. Which is bad if you e.g. always rely on events for consistency.

DEV Hint

This can be reached by registering the listeners as build in listeners instead of custom listeners. (Build in listeners will never be skipped)

Links

yanavasileva commented 1 year ago

Hi @pschalk and @zambrovski,

Thank you for raising this feature! We will evaluate it and get back to you.

Best regards, Yana

yanavasileva commented 1 year ago

Hi @tmetzke,

Assigning for a decision. Patrick and Simon already started working on a PR for the feature.

Best regards, Yana

zambrovski commented 1 year ago

To give you more context.. if you are building a library, relying on events to be fired, you have to teach your process operators to not skip listeners on any cockpit modifications. Sometimes they will still forget this, and you will end up in some potential inconsistent state. The propose is, to make it configurable inside the Camunda Properties, if the registration is in standard listeners, or in built-in listeners. So for particular customer scenarios, it is the possible just to flip the property from default (standard listeners) to built-in listeners, and the problem is gone. We assumed the simplest approach and introduced a single switch to do so. I think this is good enough instead of fine grained control if task/execution listeners are configured differently.

Check our PR for more details..

yanavasileva commented 1 year ago

Thank you, Simon. We will reach out in case of questions. Please note that there might some delay in processing the ticket due to other team's responsibilities.

ThorbenLindhauer commented 1 year ago

Hi @pschalk and @zambrovski,

I have assessed your solution proposal a bit further. Instead of making it configurable if the Spring Event Bridge listeners are treated as custom or built-in listeners, I suggest just registering them always as built-in listeners for two reasons:

The idea for the "skip custom listeners" feature is that users can choose to skip invoking custom listeners when that leads to technical issues, e.g. when the process engine cannot load the class that the BPMN model references. For all I can tell, there is no such technical issue here: either the Spring Eventing listener is registered or not.
The Spring Eventing execution listener is part of the Spring Boot starter and therefore part of the core product offering. It's only natural to treat it as a built-in listener, not as a customer (e.g. user-registered) listener

That would avoid having this extra configuration flag that users need to activate. The only reason against this solution would be that users might want to deliberately skip the Spring Eventing listener by setting the "skip custom listeners" flag in Cockpit. Do you see a possibility for this?

Cheers, Thorben

zambrovski commented 1 year ago

Hi,

'Always built-in' might also be ok, but would be a breaking change to the existing solution. Having a switch would allow not to break existing solutions.

I agree with your built-in versus custom argumentation only partly. Of course, the Spring listener is always provided and can be found by the platform, but it is more about errors produced in the listeners. So imagine the listener is built-in and invokes a Spring Event listener which is buggy and throws an error - in this case there is no way to bypass it anymore.

But in the end, in the last 10 years of using Camunda I haven't observed a single use case in which I wanted to skip the listeners. I even know about an enterprise Camunda customer that implemented a cockpit plugin flipping the default of the skip-listener checkbox (default not to skip!).

So to finalize it - I vote for integrating the listens as built-in, but I would suggest to have the property to set it to custom (maybe the default of the switch is then other then suggested in the PR, but I would like to offer the switch). If you insist on removing it - would be ok for me too.

Tell me about your decision, and if we should adjust the PR accordingly...

Cheers,

Simon

ThorbenLindhauer commented 1 year ago

Hi Simon,

I agree with your built-in versus custom argumentation only partly. Of course, the Spring listener is always provided and can be found by the platform, but it is more about errors produced in the listeners. So imagine the listener is built-in and invokes a Spring Event listener which is buggy and throws an error - in this case there is no way to bypass it anymore.

I see your point, that makes sense. So we would rather treat the Spring Event handlers as potentially failing user code. In that sense, wouldn't it be better for a Cockpit operator in any case to have the option to decide if the event handlers/listeners are triggered? I.e. even if we implement the PR as originally suggested (config flag), then activating the config flag would lead to the same situation (of course I am doing that knowingly as a user then). Or would using the flag then mean that I, as a developer, am confident enough in my custom code's robustness that I am willing to take this risk. I'm wondering if https://docs.camunda.org/manual/7.18/webapps/cockpit/extend/configuration/#skipcustomlisteners-and-skipiomappings-flags would solve the problem in your case, too, while still keeping the flexibility for the operator.

Cheers, Thorben

ThorbenLindhauer commented 1 year ago

Closing the issue due to inactivity, but feel free to respond any time and we can continue the contribution.

zambrovski commented 1 year ago

Hi Torben,

I'm a little wondered about all this. I opened you an issue we observe by two customers (and since I'm not a fan of using the paying customer axe, I'm opening a usual issue). I provided you a valid and useful implementation fixing the problem, which is in fact one existing since I provided the implemtation of the Spring Eventing Bridge in the start of Camunda Spring Boot.

Now you are closing it due to inactivity between the years, instead of just approving and merging it in? I'm confused.

Let us be more accurate in the communication:

I don't think it is only a cockpit feature, which needs to be fixed.
I want to make sure that I can control how I register listeners and if they are intended to be skipped. I'll give you an example: a listener sending a notification to the customer should be skippable by a cockpit operation if the operator decides to, but a listener used for backend task synchronization should not be - never. So I want to make sure as a developer of the solution to allow to skip the first and not skip the second. Currently, this is not possible. A user-driven listener is usually defined in the model and they should continue to be skippable, but as a developer I want to build listeners reacting on Spring Events which can be built-in nad not skappable.
I believe that introduction of the flag (set to false) will not break any of existing systems and a line in a documentation can open the ability to everyone to use it. So it is a higher quality solution then breaking existing API.
If you don't have this feature, one has to switch OFF the feature entirely and build an own Engine plugin which does exactly this. In fact I did it already in the Polyflow library because of the problem at the customer code. But this is indeed a workaround and I believe it is much better to provide this functionality in the origin - the Camunda Spring Boot integration, instead of patching it by deactivating and reimplementing it another way around.

Please tell me how can I support you in this feature. I thought I already did it and you just need to click on "merge". If there is something in the code, please point it out and I'll fix it, but don't just close the issue ... We really need this.

Cheers,

Simon

ThorbenLindhauer commented 1 year ago

Hi Simon,

Thanks for getting back to this.

I'm a little wondered about all this. I opened you an issue we observe by two customers (and since I'm not a fan of using the paying customer axe, I'm opening a usual issue). I provided you a valid and useful implementation fixing the problem, which is in fact one existing since I provided the implemtation of the Spring Eventing Bridge in the start of Camunda Spring Boot.

Now you are closing it due to inactivity between the years, instead of just approving and merging it in? I'm confused.

I hope I can clear this up a bit. Before we review, accept and merge a contribution, we want to understand that it solves a relevant problem and that we do not already have a feature in the product that solves the problem too. That's important for us as we keep code that is once merged for a very long time and we try to avoid complexity where it is not needed.

In our discussion, this was not yet clear enough for me. I still had doubts about the usefulness of the proposed solution and wanted to make you aware of another feature (configuration in Cockpit) that might also solve your problem (since you were talking about mistakes made by Cockpit operators beforehand, eliminating the possibility to make a mistake in Cockpit sounded good to me).

It's important for us to collaborate with our users in such discussions, because you observe the problem in practice where I can mostly only speculate and draw on past experiences. So anything that we can back up with practical evidence is very valuable and that is where I need your input.

In the team, we close tickets where the purpose is not fully clear yet and people stop responding. That helps us keep track of where we need to work and also avoids having tickets in the backlog that are not fully clear from our perspective. Nevertheless, whenever the conversation continues we reopen the ticket and continue to make progress.

Please tell me how can I support you in this feature. I thought I already did it and you just need to click on "merge". If there is something in the code, please point it out and I'll fix it, but don't just close the issue ... We really need this.

As said, it's most important for us that the conversation keeps going when there are still open questions.

Getting back to the feature now.

I want to make sure that I can control how I register listeners and if they are intended to be skipped. I'll give you an example: a listener sending a notification to the customer should be skippable by a cockpit operation if the operator decides to, but a listener used for backend task synchronization should not be - never. So I want to make sure as a developer of the solution to allow to skip the first and not skip the second. Currently, this is not possible. A user-driven listener is usually defined in the model and they should continue to be skippable, but as a developer I want to build listeners reacting on Spring Events which can be built-in nad not skappable.

This is an important point. It's fair to say that if a developer creates a listener with the Spring Event bridge and they make it a built-in listener, that it's their responsibility to make it robust so that it doesn't impact Cockpit operations. Whereas the feature to skip listeners via Cockpit is more important for user-provided listeners (e.g. as part of models) that may be less robust. This also explains why entirely disabling the possibility to skip listeners in Cockpit may not be the solution.

It also helps me understand that this feature is useful and will go ahead with the code review in the next days.

Cheers, Thorben

ThorbenLindhauer commented 1 year ago

Thanks for the contribution @zambrovski, @pschalk. I merged the PR in the platform. Would you be up for extending the docs, too? I think we should extend in particular the following sections: https://docs.camunda.org/manual/7.18/user-guide/spring-boot-integration/the-spring-event-bridge/ and https://docs.camunda.org/manual/7.18/user-guide/spring-boot-integration/configuration/#camunda-engine-properties.

Cheers, Thorben

zambrovski commented 1 year ago

Will do. Thank you, Torben..

zambrovski commented 1 year ago

Here you go: https://github.com/camunda/camunda-docs-manual/pull/1398 Please approve and close this issue after merge.

ThorbenLindhauer commented 1 year ago

Thanks for the PR and the contribution overall. All merged now.

camunda / camunda-bpm-platform