Remove user-provided details from various logs #1100
Conversation
| } | ||
|
|
||
| private Exception exception; |
There was a problem hiding this comment.
Why was this moved (and no longer readonly)? In C#, the normal convention is to put fields at the top of the class definition.
There was a problem hiding this comment.
It was moved without realizing (I tried undoing a bunch of changes and failed to move this to the original position). Will restore the readonly as well. Thanks for the convention note.
| } | ||
|
|
||
| private Exception? exception { get; } |
There was a problem hiding this comment.
nit: better to use a readonly field instead of a property (and move it to the top of the class definition). ALso, properties don't start with a lowercase letter.
There was a problem hiding this comment.
Thanks. I'll be following the format as the other exception variable in TaskActivityFailure, where this is readonly but not a property. Will move it to the top of the class as well.
| @@ -134,6 +134,14 @@ public OrchestrationRuntimeState(IList<HistoryEvent>? events) | |||
| /// </remarks> | |||
| public FailureDetails? FailureDetails => ExecutionCompletedEvent?.FailureDetails; | |||
|
|
|||
| /// <summary> | |||
| /// Failure that caused an orchestrator to complete, if any. | |||
| /// <remarks> | |||
There was a problem hiding this comment.
nit: <remarks> is a sibling to <summary>, not a nested child. See the FailureDetails property above for an example of how to correctly use <remarks>.
There was a problem hiding this comment.
Thanks for pointing this out. I wish Visual Studio had flagged this somehow. Im sure there's a way to set that up, but I'll leave that to another day due to time :)
There was a problem hiding this comment.
Will be fixed in next commit
Co-authored-by: Chris Gillum <[email protected]>
…b.com/Azure/durabletask into dajusto/remove-details-from-telemetry
|
Incorporated feedback, merging as such. Thanks for the prompt reviews! |
In various log events, our telemetry captures user-provided details that may contain sensitive information, particularly in the "Details" column. For safety, this PR does two things:
(1) We stop logging the reason for client operations like terminate, suspend, and resume. These are user-provided strings, and they have minimal use during investigations.
(2) When it comes to logging exceptions, we avoid logging the exception message, which may contain sensitive data. To do this, the orchestratorCompleted action now always carries a FailureDetails object, which can be inspected to obtain only the error trace and error type of an exception. This doesn't mean we do not honor the user's choice of error propagation mode, it just means that the FailureDetails property is always populated. Obviously, this may have a memory perf impact, but I found all alternatives to have tricky tradeoffs. Given that FailureDetails is positioned to be the preferred error serialization mode of future SDKs, I think it should be 'ok' to leverage it here.