-
Notifications
You must be signed in to change notification settings - Fork 48
Exposing additional integration events #2465
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
Conversation
src/ServiceControl/ExternalIntegrations/FailedMessagesUnArchivedPublisher.cs
Outdated
Show resolved
Hide resolved
src/ServiceControl/MessageFailures/Handlers/UnArchiveMessagesByRangeHandler.cs
Outdated
Show resolved
Hide resolved
src/ServiceControl/ExternalIntegrations/MessageFailureResolvedByRetryPublisher.cs
Show resolved
Hide resolved
|
I think this lacks also acceptance tests under the corresponding External integration folder. See for example |
mikeminutillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably also need to expose FailedMessageGroupArchived.
Do we know what the impact on performance is?
For Azure Service Bus this will increase the number of operations against the broker, even if there are no subscribers. Should we consider having a way to disable some/all integration events?
src/ServiceControl/ExternalIntegrations/MessageFailureResolvedByRetryPublisher.cs
Outdated
Show resolved
Hide resolved
src/ServiceControl/MessageFailures/Handlers/UnArchiveMessagesByRangeHandler.cs
Show resolved
Hide resolved
d10048e to
d232ed7
Compare
src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.PublicClr.approved.txt
Outdated
Show resolved
Hide resolved
SzymonPobiega
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions
src/ServiceControl/Recoverability/Archiving/Raven/ArchiveOperation.cs
Outdated
Show resolved
Hide resolved
src/ServiceControl/ExternalIntegrations/FailedMessageArchivedPublisher.cs
Outdated
Show resolved
Hide resolved
src/ServiceControl/ExternalIntegrations/FailedMessageGroupArchivedPublisher.cs
Outdated
Show resolved
Hide resolved
60d2360 to
4149848
Compare
...trol.AcceptanceTests/Recoverability/ExternalIntegration/When_a_failed_message_is_archived.cs
Outdated
Show resolved
Hide resolved
...ptanceTests/Recoverability/ExternalIntegration/When_a_failed_message_is_resolved_by_retry.cs
Outdated
Show resolved
Hide resolved
...ptanceTests/Recoverability/ExternalIntegration/When_a_failed_message_is_resolved_by_retry.cs
Outdated
Show resolved
Hide resolved
src/ServiceControl/Recoverability/Archiving/ArchiveAllInGroupHandler.cs
Outdated
Show resolved
Hide resolved
mikeminutillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all this looks good. My chief concerns would be:
- Publishing the results of bulk opertaions with a list of GUIDs could quickly create very large messages. We should maybe consider batching these at the integration event level.
- Publishing more events may incur more costs in cloud environments. We should look at providing options to disable some/all of these event types.
...trol.AcceptanceTests/Recoverability/ExternalIntegration/When_a_failed_message_is_archived.cs
Outdated
Show resolved
Hide resolved
We have changed the code to publish these messages for each archived batch (1000 docs) so we are safe here that we won't cross the threshold even on the most restricted transport (ASQ) |
I think we are not making things significantly worse because the biggest impact on cost is probably |
My concern is that for every |
One more event is a situation that was already there before we added new events. Secondly, these events won't get published unless there is an integration event subscriber in the system. |
I am not following this. Can you explain how this situation was already present?
For message-driven pub-sub that is true. For native-pub sub transports like Azure Service Bus (where every operation costs money) we still are going to push the event to the topic, even if there are no subscribers for it. |
|
From Slack To summarize the conversation:
The outcome is that we will introduce a config flag to turn off the publishing of integration events.
|
5ba6824 to
1c1803c
Compare
danielmarbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments
...trol.AcceptanceTests/Recoverability/ExternalIntegration/When_a_failed_message_is_archived.cs
Outdated
Show resolved
Hide resolved
src/ServiceControl/ExternalIntegrations/MessageFailureResolvedByRetryPublisher.cs
Outdated
Show resolved
Hide resolved
src/ServiceControl/ExternalIntegrations/MessageFailureResolvedManuallyPublisher.cs
Outdated
Show resolved
Hide resolved
|
@danielmarbach can you have a look now? Thx. |
…Frontend/eslint-9.29.0 Bump eslint from 9.28.0 to 9.29.0 in /src/Frontend
#1445