Skip to content

Include Exception Properties at FailureDetails - #474

Merged
Naiyuan Tian (nytian) merged 15 commits into
mainfrom
nytian/failure-details-excep
Oct 13, 2025
Merged

Include Exception Properties at FailureDetails#474
Naiyuan Tian (nytian) merged 15 commits into
mainfrom
nytian/failure-details-excep

Conversation

@nytian

@nytianNaiyuan Tian (nytian) commented Oct 2, 2025

Copy link
Copy Markdown
Contributor

This PR supports register custom provider that help include exception properties at TaskFailureDetails. Supports include running with both DurableTaskWorker and Durable Functions (.NET Isolated).

Note: related PR :

@nytianNaiyuan Tian (nytian) changed the title Include Exception Properties at FailueDetailsInclude Exception Properties at FailureDetailsOct 2, 2025
}
}

static P.TaskFailureDetails? GetFailureDetails(FailureDetails? failureDetails)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

remove this because we will call the one at ptobufutils instead

Comment threadsrc/Worker/Core/IExceptionPropertiesProvider.cs
exception.StackTrace,
FromExceptionRecursive(exception.InnerException));
FromExceptionRecursive(exception.InnerException),
null);// might need to udpate this later

@nytianNaiyuan Tian (nytian)Oct 2, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Question for the public static TaskFailureDetails FromException(Exception exception)

I checked all the reference, and it seems all the cases we called this is when the exception is a DTCore.OrchestrationExcep, which means it should already have properties included. Thus currently we don't need to extract properties here.

But I am not sure if there might be a case when the exception is not from dt.core and this return new FailureDetails block is reached. If so, then we need to extract properties in this repo, which means I need to update OrchestrationContextWrapper class etc. But I didn't find a case like this from current code path. Let me know if there is any other thought on this.

? new(new(p.Name), p.OrchestrationInstance.InstanceId)
: null;

DurableTaskShimFactory factory = services is null

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I only updated line 211 to 226.. the whole file changed because of the VS outlining thing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was able to read it properly if I turned off whitespace in the diff view. This can happen sometimes if VS or the git client changes the line endings.

@cgillumChris Gillum (cgillum) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few initial comments. Haven't reviewed everything yet.

Comment threadsrc/Abstractions/TaskFailureDetails.cs
Comment threadsrc/Grpc/versions.txt Outdated
@@ -1,2 +1,2 @@
# The following files were downloaded from branch main at 2025-09-17 01:45:58 UTC
https://raw.githubusercontent.com/microsoft/durabletask-protobuf/f5745e0d83f608d77871c1894d9260ceaae08967/protos/orchestrator_service.proto
# The following files were downloaded from branch nytian/failure-details at 2025-10-01 21:51:24 UTC

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reminder to update this to main once the protobuf changes in durabletask-protobuf are merged.

Comment threadsrc/Shared/Grpc/ProtoUtils.cs Outdated
Comment threadsrc/Shared/Grpc/ProtoUtils.cs Outdated
Comment threadsrc/Shared/Grpc/ProtoUtils.cs Outdated
Comment threadsrc/Abstractions/TaskFailureDetails.cs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, apart from the note about updating the protobuf definitions once that related change is committed.

@cgillumChris Gillum (cgillum) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I finished reviewing everything and have a few more minor comments.

/// </remarks>
public class DurableTaskShimFactory
{
public readonly IExceptionPropertiesProvider? exceptionPropertiesProvider;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was making this public an accident? We should never have public fields.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

oh thanks for reminding. I added these at first because I thought we need this when using with durable functions isolated. But the test turns out actually we don't need to propagate provider from worker extensions to here. I just deleted these.

Comment threadsrc/Worker/Core/IExceptionPropertiesProvider.cs
Comment threadsrc/Worker/Grpc/GrpcDurableTaskWorker.cs
Comment threadtest/Grpc.IntegrationTests/GrpcSidecar/Grpc/ProtobufUtils.cs Outdated
? new(new(p.Name), p.OrchestrationInstance.InstanceId)
: null;

DurableTaskShimFactory factory = services is null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was able to read it properly if I turned off whitespace in the diff view. This can happen sometimes if VS or the git client changes the line endings.

Comment threadsrc/Shared/Grpc/ProtoUtils.cs Outdated
if (orchestrationActivity is null)
{
return null;
return DateTime.Parse(stringValue[3..], CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment about TryParse instead of Parse, though I wonder why we have this same logic in two places?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The grpc integration test will not use the ProtoUtils under folder src/shared/grpc but will use the one at it's own folder which is test/Grpc.IntegrationTests/GrpcSidecar/Grpc/ProtobufUtils.cs

@nytian
Naiyuan Tian (nytian) merged commit 6a1f434 into mainOct 13, 2025
4 checks passed
@nytian
Naiyuan Tian (nytian) deleted the nytian/failure-details-excep branch October 13, 2025 18:44
Sign up for freeto 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.

3 participants

@nytian@cgillum@philliphoff