Skip to content

Add structured abandonment details to Azure Storage logs - #1395

Open
wangbill (YunchuWang) wants to merge 5 commits into
mainfrom
yunchuwang-add-abandonment-details
Open

Add structured abandonment details to Azure Storage logs#1395
wangbill (YunchuWang) wants to merge 5 commits into
mainfrom
yunchuwang-add-abandonment-details

Conversation

@YunchuWang

@YunchuWang wangbill (YunchuWang) commented Sep 3, 2026

Copy link
Copy Markdown
Member

Summary

  • Add structured Details to Azure Storage AbandoningMessage telemetry and thread code-authored explanations through all queue/session abandonment paths.
  • Preserve the existing human-readable AbandoningMessage text exactly so customers that parse rendered log messages remain compatible.
  • Append Details to AnalyticsEventSource event 104 and increment its version from 7 to 8.
  • Add focused coverage for structured fields, unchanged formatted text, propagation, EventSource schema validation, and positional payload ordering.

Compatibility

This is a telemetry-only change. Details is available through structured ILogger state and EventSource, while FormattedMessage/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 Details field. 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 passed
  • dotnet test Test\DurableTask.AzureStorage.Tests\DurableTask.AzureStorage.Tests.csproj -f net48 --no-restore --filter "FullyQualifiedName~LoggingTests|Name=ValidateEventSource" — 5 passed
  • dotnet build src\DurableTask.AzureStorage\DurableTask.AzureStorage.csproj -c Release --no-restore — 0 warnings, 0 errors
  • git diff --check origin/main...HEAD

Related: Azure/azure-functions-durable-extension#1608

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2129cc7b-4cef-4aae-93b7-076bbf462d82
Copilot AI lite review requested due to automatic review settings September 3, 2026 20:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 Details field to AbandoningMessage structured logs and EventSource event 104 (version bumped to 8).
  • Threads a details string 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.

Comment on lines +26 to +31
[TestClass]
public class LoggingTests
{
[TestMethod]
public void AbandoningMessage_HasExpectedStructuredFieldsAndMessage()
{
Comment on lines +62 to +64
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);
Comment on lines 196 to +200
PopReceipt ?? string.Empty,
VisibilityTimeoutSeconds,
AppName,
ExtensionVersion);
ExtensionVersion,
Details);
Comment on lines +398 to +404
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>
Copilot AI review requested due to automatic review settings September 3, 2026 20:57
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2129cc7b-4cef-4aae-93b7-076bbf462d82

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 under test/ (lowercase) and Test/ 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 into test/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 is VisibilityTimeoutSeconds and the value passed here (numSecondsToWait) is seconds (used with TimeSpan.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

Copilot AI review requested due to automatic review settings September 3, 2026 21:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 under test/ (see DurableTask.sln referencing test\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 the test/DurableTask.AzureStorage.Tests/ project (or that project is updated to include this file).
namespace DurableTask.AzureStorage.Tests

src/DurableTask.AzureStorage/AnalyticsEventSource.cs:200

  • WriteEvent is currently passed Details without a null-coalesce, while other string parameters (e.g., ExecutionId, PopReceipt) are normalized to string.Empty. If Details is ever null at runtime, EventSource will throw when writing this event. Consider normalizing it to string.Empty for 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>
Copilot AI review requested due to automatic review settings September 3, 2026 21:18
@YunchuWang wangbill (YunchuWang) changed the title Add details to abandoned-message logs Add abandonment details to Azure Storage message logs Sep 3, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@YunchuWang wangbill (YunchuWang) changed the title Add abandonment details to Azure Storage message logs Add structured abandonment details to Azure Storage logs Sep 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 under test/DurableTask.AzureStorage.Tests/ (lowercase) and there is no corresponding .csproj under Test/. 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 VisibilityTimeoutSeconds as 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

Copilot AI review requested due to automatic review settings September 3, 2026 21:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 under test/ (lowercase) and Test/ has no .csproj files. 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 into test/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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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:42 references lowercase test/DurableTask.AzureStorage.Tests, while this file is under uppercase Test/, and the project has no external Compile include. Consequently, these four telemetry tests are not built or run in Linux CI. Move the file into test/DurableTask.AzureStorage.Tests/.
    [TestClass]
    public class LoggingTests
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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