-
Notifications
You must be signed in to change notification settings - Fork 285
Include Exception Properties at FailureDetails via Custom Provider #3215
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
andystaples
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.
Looking mostly good - just one thing to discuss
test/e2e/Apps/BasicDotNetIsolated/CustomExceptionPropertiesOrchestration.cs
Show resolved
Hide resolved
test/e2e/Apps/BasicDotNetIsolated/CustomExceptionPropertiesOrchestration.cs
Outdated
Show resolved
Hide resolved
cgillum
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.
I'm a bit confused about why we mix JSON and protobuf together in the same payloads. I don't remember us doing this before, so maybe I'm missing some context.
andystaples
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.
Comments
test/e2e/Apps/BasicDotNetIsolated/CustomExceptionPropertiesOrchestration.cs
Outdated
Show resolved
Hide resolved
| JValue jv => ConvertJValueToValue(jv), | ||
| JArray ja => Value.ForList(ja.Select(ConvertObjectToValue).ToArray()), | ||
| JObject jo => Value.ForStruct(new Struct | ||
| { | ||
| Fields = { jo.Properties().ToDictionary(p => p.Name, p => ConvertObjectToValue(p.Value)) }, | ||
| }), |
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.
Couple of questions about this
- Why Newtonsoft objects specifically, and not other JSON library objects?
- I notice we are not doing these Newtonsoft conversions in TaskFailureDetailsConverter.cs - is this deliberate?
- Also, this may be generally unintuitive for customers who expect their JValue properties to be returned as JValue (or the string equivalent) and instead get some defined object.
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 added these because, in the test case, some nested objects such as lists and dictionaries are serialized and deserialized as JSON values when passed from Worker.Extensions to WebJobs.Extensions. Therefore, we need to include this JSON block here.
For TaskFailureDetailsConverter, there’s no serialization or deserialization involved — it directly handles the object itself, so a JSON block isn’t needed there.
Yeah, if they specifically expect a JValue, we might be violating that here — but are there really many such cases? It seems that handling nested structures is more important overall. I’m fine with either approach, though. leave it as it-is for other folks to join the discussion..
Issue describing the changes in this PR
Add support for including exception properties at TaskFailureDetails via custom provider
Note: This PR won't work unless microsoft/durabletask-dotnet#474 is merged.
Pull request checklist
pending_docs.mdrelease_notes.md/src/Worker.Extensions.DurableTask/AssemblyInfo.csdevandmainbranches and will not be merged into thev2.xbranch.