-
Notifications
You must be signed in to change notification settings - Fork 325
Add Poison Message Handling to the Dispatchers #1331
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
base: main
Are you sure you want to change the base?
Changes from all commits
de244ff
8f6bf9d
ea5a36f
3ab0859
4c7665d
3bd1dc9
2e3c7c2
eecc077
f0fc35d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,20 @@ protected HistoryEvent(int eventId) | |
| [DataMember] | ||
| public virtual EventType EventType { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the number of times this event has been dispatched. | ||
| /// </summary> | ||
| [DataMember] | ||
| public int DispatchCount { get; set; } | ||
sophiatev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// <summary> | ||
| /// Gets or sets whether or not this event has been marked as "poisoned". | ||
| /// This can occur if the event's dispatch count exceeds a certain threshold, | ||
| /// or if some other error occurs during dispatch. | ||
| /// </summary> | ||
| [DataMember] | ||
| public bool IsPoisoned { get; set; } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we really want this |
||
|
|
||
| /// <summary> | ||
| /// Implementation for <see cref="IExtensibleDataObject.ExtensionData"/>. | ||
| /// </summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1976,5 +1976,54 @@ void IEventSourceEvent.WriteEventSource() => | |
| Utils.PackageVersion); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Log event representing the discarding of a "poison" message. | ||
| /// </summary> | ||
| internal class PoisonMessageDetected : StructuredLogEvent, IEventSourceEvent | ||
| { | ||
| public PoisonMessageDetected(OrchestrationInstance orchestrationInstance, string eventType, int taskEventId, string details) | ||
| { | ||
| this.InstanceId = orchestrationInstance?.InstanceId ?? string.Empty; | ||
| this.ExecutionId = orchestrationInstance?.ExecutionId ?? string.Empty; | ||
| this.EventType = eventType; | ||
| this.TaskEventId = taskEventId; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also include |
||
| this.Details = details; | ||
| } | ||
sophiatev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| [StructuredLogField] | ||
| public string InstanceId { get; } | ||
|
|
||
| [StructuredLogField] | ||
| public string ExecutionId { get; } | ||
|
|
||
| [StructuredLogField] | ||
| public string EventType { get; } | ||
|
|
||
| [StructuredLogField] | ||
| public int TaskEventId { get; } | ||
|
|
||
| [StructuredLogField] | ||
| public string Details { get; } | ||
|
|
||
| public override EventId EventId => new EventId( | ||
| EventIds.PoisonMessageDetected, | ||
| nameof(EventIds.PoisonMessageDetected)); | ||
|
|
||
| public override LogLevel Level => LogLevel.Error; | ||
|
|
||
| protected override string CreateLogMessage() => | ||
| $"{this.InstanceId}: Poison message detected for {GetEventDescription(this.EventType, this.TaskEventId)}: {this.Details}"; | ||
|
|
||
| void IEventSourceEvent.WriteEventSource() => | ||
| StructuredEventSource.Log.PoisonMessageDetected( | ||
| this.InstanceId, | ||
| this.ExecutionId, | ||
| this.EventType, | ||
| this.TaskEventId, | ||
| this.Details, | ||
| Utils.AppName, | ||
| Utils.PackageVersion); | ||
| } | ||
|
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,13 +13,15 @@ | |
| #nullable enable | ||
| namespace DurableTask.Core.Logging | ||
| { | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Text; | ||
| using DurableTask.Core.Command; | ||
| using DurableTask.Core.Common; | ||
| using DurableTask.Core.Entities.EventFormat; | ||
| using DurableTask.Core.Entities.OperationFormat; | ||
| using DurableTask.Core.History; | ||
| using Microsoft.Extensions.Logging; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Text; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we revert the sort order change to this file? Normally we keep |
||
|
|
||
| class LogHelper | ||
| { | ||
|
|
@@ -760,6 +762,42 @@ internal void RenewActivityMessageFailed(TaskActivityWorkItem workItem, Exceptio | |
| this.WriteStructuredLog(new LogEvents.RenewActivityMessageFailed(workItem, exception), exception); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Logs that a "poison message" has been detected and is being dropped. | ||
| /// </summary> | ||
| /// <param name="orchestrationInstance">The orchestration instance this event was sent to.</param> | ||
| /// <param name="historyEvent">The "poisoned" event.</param> | ||
| /// <param name="details">Extra details related to the processing of this poison message.</param> | ||
| internal void PoisonMessageDetected(OrchestrationInstance? orchestrationInstance, HistoryEvent historyEvent, string details) | ||
| { | ||
| if (this.IsStructuredLoggingEnabled) | ||
| { | ||
| this.WriteStructuredLog(new LogEvents.PoisonMessageDetected( | ||
| orchestrationInstance, | ||
sophiatev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| historyEvent.EventType.ToString(), | ||
| Utils.GetTaskEventId(historyEvent), | ||
| details)); | ||
| } | ||
sophiatev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /// <summary> | ||
| /// Logs that a "poison" entity request message has been detected and is being dropped. | ||
| /// </summary> | ||
| /// <param name="orchestrationInstance">The orchestration instance this event was sent to.</param> | ||
| /// <param name="requestMessage">The "poisoned" request message.</param> | ||
| /// <param name="details">Extra details related to the processing of this poison message.</param> | ||
| internal void PoisonMessageDetected(OrchestrationInstance orchestrationInstance, RequestMessage requestMessage, string details) | ||
| { | ||
| if (this.IsStructuredLoggingEnabled) | ||
| { | ||
| this.WriteStructuredLog(new LogEvents.PoisonMessageDetected( | ||
| orchestrationInstance, | ||
| requestMessage.IsLockRequest ? "LockRequest" : "OperationRequest", | ||
| taskEventId: -1, | ||
| details)); | ||
| } | ||
| } | ||
| #endregion | ||
|
|
||
| internal void OrchestrationDebugTrace(string instanceId, string executionId, string details) | ||
|
|
||
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.
I suggest we have these new properties be omitted by default if not populated, especially
IsPoisoned. This will reduce the chance that there's some compatibility issue with older versions of the SDK that try to deserialize the history.