Add structured abandonment details to Azure Storage logs - #1395
Add structured abandonment details to Azure Storage logs#1395wangbill (YunchuWang) wants to merge 5 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2129cc7b-4cef-4aae-93b7-076bbf462d82
There was a problem hiding this comment.
🟡 Changes recommended
The added tests are placed under Test/ (not the runnable test/ project directory) and the formatted log message currently mislabels a seconds-based delay as milliseconds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new Details field to the Azure Storage “AbandoningMessage” telemetry (structured log + formatted message + EventSource), and propagates consistent, code-authored abandonment reasons through internal call paths.
Changes:
- Appends a
Detailsfield toAbandoningMessagestructured logs and EventSource event 104 (version bumped to 8). - Threads a
detailsstring through internal abandonment APIs/call sites (session release, out-of-order messages, deferred execution-start messages, etc.). - Adds new tests intended to validate structured fields, formatting, and EventSource schema.
File summaries
| File | Description |
|---|---|
| Test/DurableTask.AzureStorage.Tests/LoggingTests.cs | Adds tests for AbandoningMessage fields/format/schema/propagation (but see review comments about location). |
| src/DurableTask.AzureStorage/OrchestrationSessionManager.cs | Propagates a concrete abandonment detail for deferred execution-start messages. |
| src/DurableTask.AzureStorage/Messaging/TaskHubQueue.cs | Adds details parameter to abandonment path and includes it in AbandoningMessage logging. |
| src/DurableTask.AzureStorage/Messaging/ControlQueue.cs | Propagates details for abandonment when deserialization fails and through overrides. |
| src/DurableTask.AzureStorage/Logging/LogHelper.cs | Extends AbandoningMessage helper to accept and forward details. |
| src/DurableTask.AzureStorage/Logging/LogEvents.cs | Adds Details structured field and appends it to the formatted log message + EventSource payload. |
| src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs | Threads abandonment details through session/message abandonment paths. |
| src/DurableTask.AzureStorage/AnalyticsEventSource.cs | Updates AbandoningMessage ETW event to version 8 and appends Details to payload. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| [TestClass] | ||
| public class LoggingTests | ||
| { | ||
| [TestMethod] | ||
| public void AbandoningMessage_HasExpectedStructuredFieldsAndMessage() | ||
| { |
| Assert.AreEqual( | ||
| "instance-id: Abandoning [TaskScheduled#42] message back to control-queue and setting a visibility delay of 30ms: The activity work item could not be processed.", | ||
| ((ILogEvent)logEvent).FormattedMessage); |
| PopReceipt ?? string.Empty, | ||
| VisibilityTimeoutSeconds, | ||
| AppName, | ||
| ExtensionVersion); | ||
| ExtensionVersion, | ||
| Details); |
| protected override string CreateLogMessage() => string.Format( | ||
| "{0}: Abandoning {1} message back to {2} and setting a visibility delay of {3}ms", | ||
| "{0}: Abandoning {1} message back to {2} and setting a visibility delay of {3}ms: {4}", | ||
| this.InstanceId, | ||
| GetEventDescription(this.EventType, this.TaskEventId), | ||
| this.PartitionId, | ||
| this.VisibilityTimeoutSeconds); | ||
| this.VisibilityTimeoutSeconds, | ||
| this.Details); |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2129cc7b-4cef-4aae-93b7-076bbf462d82
There was a problem hiding this comment.
🔵 Needs a closer look
The newly added tests are placed under Test/ (not compiled/run by the solution’s test/ projects), and the AbandoningMessage formatted output currently mislabels seconds as milliseconds.
Review details
Suppressed comments (3)
Test/DurableTask.AzureStorage.Tests/LoggingTests.cs:24
- This new test file is under
Test/(uppercase), but the solution and active test projects are undertest/(lowercase) andTest/currently has no referenced.csproj. As-is, these tests won’t be compiled or run, and the test command in the PR description (Test\DurableTask.AzureStorage.Tests\...) won’t resolve. Move this file intotest/DurableTask.AzureStorage.Tests/(or update the solution/project structure accordingly) so CI actually executes the coverage you’re adding.
namespace DurableTask.AzureStorage.Tests
{
using System;
using System.Collections.Generic;
using System.Diagnostics.Tracing;
using System.Linq;
using System.Reflection;
using DurableTask.AzureStorage.Logging;
using DurableTask.Core.Logging;
using Microsoft.Extensions.Logging;
using Microsoft.VisualStudio.TestTools.UnitTesting;
src/DurableTask.AzureStorage/Logging/LogEvents.cs:400
- The formatted message says the visibility delay is in
ms, but the structured field isVisibilityTimeoutSecondsand the value passed here (numSecondsToWait) is seconds (used withTimeSpan.FromSeconds). This makes the log message misleading for operators.
protected override string CreateLogMessage() => string.Format(
"{0}: Abandoning {1} message back to {2} and setting a visibility delay of {3}ms",
this.InstanceId,
Test/DurableTask.AzureStorage.Tests/LoggingTests.cs:64
- This expected formatted message uses
30ms, but the visibility timeout field/value is expressed in seconds. If the production log message is corrected to seconds, update the test expectation accordingly.
Assert.AreEqual(
"instance-id: Abandoning [TaskScheduled#42] message back to control-queue and setting a visibility delay of 30ms",
((ILogEvent)logEvent).FormattedMessage);
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new test file is currently under Test/ instead of the active test/ project tree, so coverage likely won’t run in CI on case-sensitive filesystems.
Review details
Suppressed comments (2)
Test/DurableTask.AzureStorage.Tests/LoggingTests.cs:14
- This new test file is under
Test/, but the solution and active test project are undertest/(see DurableTask.sln referencingtest\DurableTask.AzureStorage.Tests\DurableTask.AzureStorage.Tests.csproj). As a result, this test won't be compiled or executed in CI on case-sensitive filesystems unless it’s moved into thetest/DurableTask.AzureStorage.Tests/project (or that project is updated to include this file).
namespace DurableTask.AzureStorage.Tests
src/DurableTask.AzureStorage/AnalyticsEventSource.cs:200
WriteEventis currently passedDetailswithout a null-coalesce, while other string parameters (e.g.,ExecutionId,PopReceipt) are normalized tostring.Empty. IfDetailsis ever null at runtime, EventSource will throw when writing this event. Consider normalizing it tostring.Emptyfor consistency and robustness.
PopReceipt ?? string.Empty,
VisibilityTimeoutSeconds,
AppName,
ExtensionVersion,
Details);
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new tests are added under Test/ (not referenced by the solution/test projects) so they won’t run, and the updated formatted message incorrectly labels a seconds value as milliseconds.
Review details
Suppressed comments (2)
Test/DurableTask.AzureStorage.Tests/LoggingTests.cs:16
- This new test file is under
Test/(capital T), but DurableTask.sln references the active AzureStorage test project undertest/DurableTask.AzureStorage.Tests/(lowercase) and there is no corresponding.csprojunderTest/. As a result, these tests won’t be compiled or executed in the normal build/test flow.
namespace DurableTask.AzureStorage.Tests
{
using System;
src/DurableTask.AzureStorage/Logging/LogEvents.cs:399
- The formatted log message labels
VisibilityTimeoutSecondsas milliseconds ({3}ms), but the value is computed as seconds (e.g.,TimeSpan.FromSeconds(numSecondsToWait)in TaskHubQueue). This makes the log output misleading.
"{0}: Abandoning {1} message back to {2} and setting a visibility delay of {3}ms",
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The newly added tests are under Test/ (not test/) and are therefore unlikely to be included in the active test projects/CI, undermining the PR’s stated validation and coverage.
Review details
Suppressed comments (1)
Test/DurableTask.AzureStorage.Tests/LoggingTests.cs:28
- This test file was added under
Test/, but the solution/test projects in this repo are undertest/(lowercase) andTest/has no.csprojfiles. As a result, these new tests likely won't be compiled or run in CI, which undermines the PR's stated validation/coverage goals. Move this file intotest/DurableTask.AzureStorage.Tests/(or otherwise include it in a referenced test project) so the added assertions actually execute.
namespace DurableTask.AzureStorage.Tests
{
using System;
using System.Collections.Generic;
using System.Diagnostics.Tracing;
using System.Linq;
using System.Reflection;
using DurableTask.AzureStorage.Logging;
using DurableTask.Core.Logging;
using Microsoft.Extensions.Logging;
using Microsoft.VisualStudio.TestTools.UnitTesting;
[TestClass]
public class LoggingTests
{
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new tests reside under uppercase Test/ and therefore are not compiled by the active lowercase test/ project on case-sensitive systems.
Review details
Suppressed comments (1)
Test/DurableTask.AzureStorage.Tests/LoggingTests.cs:27
- This test class is outside the active test project on case-sensitive systems:
DurableTask.sln:42references lowercasetest/DurableTask.AzureStorage.Tests, while this file is under uppercaseTest/, and the project has no externalCompileinclude. Consequently, these four telemetry tests are not built or run in Linux CI. Move the file intotest/DurableTask.AzureStorage.Tests/.
[TestClass]
public class LoggingTests
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Summary
Detailsto Azure StorageAbandoningMessagetelemetry and thread code-authored explanations through all queue/session abandonment paths.AbandoningMessagetext exactly so customers that parse rendered log messages remain compatible.Detailsto AnalyticsEventSource event 104 and increment its version from 7 to 8.Compatibility
This is a telemetry-only change.
Detailsis available through structuredILoggerstate and EventSource, whileFormattedMessage/ToString()remains unchanged. The change preserves abandon/delete decisions, queue operations, visibility timeout/backoff, retry timing, event ID 104, the existing Warning level, and all existing EventSource payload positions. No public provider contract or discard-event schema is changed. Detail values are code-authored strings and do not include queue payloads, user input, secrets, exception messages, or stack traces.Telemetry rollout note
Metadata-driven EventSource consumers will discover the appended field automatically. A downstream ETW, Geneva, or Kusto mapping explicitly pinned to event 104 version 7 and its 13-field payload must add a version 8 mapping for the appended
Detailsfield. This is an ingestion-schema consideration only; it does not affect Durable Task application behavior.Validation
dotnet test Test\DurableTask.AzureStorage.Tests\DurableTask.AzureStorage.Tests.csproj -f net8.0 --no-restore --filter "FullyQualifiedName~LoggingTests|Name=ValidateEventSource"— 5 passeddotnet test Test\DurableTask.AzureStorage.Tests\DurableTask.AzureStorage.Tests.csproj -f net48 --no-restore --filter "FullyQualifiedName~LoggingTests|Name=ValidateEventSource"— 5 passeddotnet build src\DurableTask.AzureStorage\DurableTask.AzureStorage.csproj -c Release --no-restore— 0 warnings, 0 errorsgit diff --check origin/main...HEADRelated: Azure/azure-functions-durable-extension#1608