Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add DomainEvent activity #5994

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cfgiugliano
Copy link
Contributor

@cfgiugliano cfgiugliano commented Oct 2, 2024

This adds the DomainEvent activity as proposed in #5583


This change is Reviewable

@cfgiugliano
Copy link
Contributor Author

@dotnet-policy-service agree

@sfmskywalker
Copy link
Member

Would it make sense to use INotificationSender instead of IWebhookDispatcher?

@cfgiugliano
Copy link
Contributor Author

cfgiugliano commented Oct 3, 2024

This was added to provide feature parity with the RunTask and I thought this was just a default notification handler, but what would be the reason for this additional indirection of a notification publishing another notification?

@cfgiugliano cfgiugliano marked this pull request as ready for review October 3, 2024 20:09
@sfmskywalker
Copy link
Member

I was more thinking that we want to be able to publish domain events, which are "lower-level" than webhook events. Ergo, it doesn't make sense to me to use a service called "webhook dispatcher" when it has nothing to do with webhooks. For that reason, I'm thinking that using INoticationSender is more suitable. The webhook dispatcher uses this too under the hood. Alternatively, we could consider adding a new service called e.g. IDomainEventDispatcher (which would probably still use INotificationSender, just like IWebhookDispatcher).

@cfgiugliano
Copy link
Contributor Author

But the activity execution is already publishing a the domain event as notification using INotificationSender (this could be chanced to consume a IDomainEventDispatcher, yes) but the current handler is just a default implementation of the notification handler like for RunTask. If required, it is still possible to implement a custom handler and handle the domain event with custom logic

@cfgiugliano cfgiugliano force-pushed the feature/domain-event-notification-activity branch from 66cc646 to 31e8b94 Compare February 8, 2025 10:22
@cfgiugliano
Copy link
Contributor Author

I've added a IDomainEventDispatcher with both a background and synchronous implementation, and exposed a factory in WorfklowsRuntimeFeature just like for the ITaskDispatcher, then updated the handler under webhook.

@sfmskywalker
Copy link
Member

Hi @cfgiugliano, thank you for all the nice work!

I need more time to go through the PR, but I'll offer som initial thoughts.

  • I am not a fan of having a separate DomainEvent and an Event, because Event serves the same purpose and having two models that are conceptually the same but have a different effect does not seem all to clean.
  • Instead, I would explore the possibility of refactoring the PublishEvent activity to use the mediator and publish a notification.
  • One handler would then attempt to resume any event bookmarks
  • Another handler, living in the Webhooks module, would deliver the event to any & all sinks by invoking the webhook dispatcher using the event name as the webhook name.

I think this would reduce some redundancy while still being able to deliver events to webhook sinks.
Alternatively, I would also be open to the idea of having a "webhooks only" model, where we would have the following things:

  • a PublishWebhookEvent activity
  • a WebhookEvent model carrying the webhook event name + payload.

A potential benefits of this specialized API might be:

  • Focused problem domain
  • No performance degradation, like with attempting to publish Events as Webhooks and resuming Event activities, which add overhead even if there are no workflows waiting for this event and there are no webhooks listening. That being said, this of course would largely depend on how "smart" the notification handlers are implemented by minimizing overhead.

And perhaps we could have both. It's nice to have options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants