Add tracing for System.Transactions #13185
Conversation
|
Hi @qizhanMS, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
@jimcarley are you the owner of the System.Transactions area? |
|
@qizhanMS, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| _cloneId = 1; | ||
| if (DiagnosticTrace.Information) | ||
| TransactionsEtwProvider etwLog = TransactionsEtwProvider.Log; | ||
| if (etwLog.IsEnabled()) |
There was a problem hiding this comment.
would it be cleaner to move the IsEnabled check inside the etwLog methods, so it's all one line? it's no more function calls.
There was a problem hiding this comment.
would it be cleaner to move the IsEnabled check inside the etwLog methods, so it's all one line? it's no more function calls.
That's likely fine in this particular case. It's less fine in cases where the call site to the tracing operation does any measurable amount of work, or if the tracing method itself can't or won't be inlined (the IsEnabled check will be).
| if (etwLog.IsEnabled()) | ||
| { | ||
| TransactionCreatedTraceRecord.Trace(SR.TraceSourceLtm, TransactionTraceId); | ||
| etwLog.TransactionCreated(this, "CommittableTransaction"); |
There was a problem hiding this comment.
You might consider taking advantage of some newer C# features, like [CallerMemberName], [CallerLineNumber], etc. I recently did so in #13134 if you're looking for an example.
There was a problem hiding this comment.
Do you know which value to use for "thisOrContextObject" when "this" is not available, such as in static method or static class?
There was a problem hiding this comment.
Do you know which value to use for "thisOrContextObject" when "this" is not available, such as in static method or static class?
It's up to you. I named it that for the networking logging as often the right value was just null, but sometimes functions were static but had an argument that was sort of the logical this, some object that represented the overall state for the operation, a "context" object, so I named it "thisOrContextObject" so that the code could pass in this, null, or whatever else made sense.
|
|
||
| namespace System.Transactions.Diagnostics | ||
| { | ||
| /* |
There was a problem hiding this comment.
Just delete these files instead of commenting them out.
| EnlistmentType.PromotableSinglePhase, | ||
| EnlistmentOptions.None | ||
| ); | ||
| etwLog.TransactionstateEnlist(tx._durableEnlistment.EnlistmentTraceId, "PromotableSinglePhase", "None"); |
There was a problem hiding this comment.
I'd prefer the ETW event method takes the EnlistmentType and EnlistmentOptions enums as parameters and calls ToString() on the values rather than passing in the strings "PromotableSinglePhase" and "None".
| // Transactions Events | ||
| // | ||
|
|
||
| #region Transcation Creation |
| } | ||
| #endregion | ||
|
|
||
| #region Transcation Dependent Clone Complete |
|
|
||
| #region Enlistment Commited | ||
| /// <summary>Trace an enlistment that commited.</summary> | ||
| /// <param name="enlistment">The enlistment aborted.</param> |
There was a problem hiding this comment.
param should be "The enlistment that committed"
| /// <summary>Trace an enlistment that commited.</summary> | ||
| /// <param name="enlistment">The enlistment aborted.</param> | ||
| [NonEvent] | ||
| internal void EnlistmentCommited(InternalEnlistment enlistment) |
There was a problem hiding this comment.
Please search/replace "Commited" with "Committed".
| public const EventOpcode ForceRollback = (EventOpcode)102; | ||
| public const EventOpcode Incomplete = (EventOpcode)123; | ||
| public const EventOpcode InDoubt = (EventOpcode)105; | ||
| public const EventOpcode IntrenalError = (EventOpcode)124; |
There was a problem hiding this comment.
Well, change IntrenalError to InternalError. You have to pick whether to order the properties alphabetically or by opcode number. You can't do both because when the list is modified in the future, it will be out of order.
There was a problem hiding this comment.
Done with the typo. For the ordering, we can do alphabetically and if we need to add more, we just update all eventOpcode after the inserted one.
| public const EventOpcode Commited = (EventOpcode)104; | ||
| public const EventOpcode Create = (EventOpcode)100; | ||
| public const EventOpcode Created = (EventOpcode)119; | ||
| public const EventOpcode CurentChanged = (EventOpcode)120; |
There was a problem hiding this comment.
Change CurentChanged to CurrentChanged. Same not as above regarding the order.
|
It looks like you have removed the TraceSource* distinction from each event. From what I could find there's Base, Ltm, and Distributed. This data might be useful for filtering so consider creating keywords for each. [QizhanMS's response] We already use Task and Opcode in the ETW tracing, user can easily check the event they are interested in. If we use Base, Ltm and Distributed in the event message, we can help user to narrow down a lit bit of their events. The trade off is a lit bit longer message. I am ok to add it. [QizhanMS's update] using TraceSource* and code change is in commit #3 |
|
Qi and I are both working on it, yes. In reply to: 257407084 [](ancestors = 257407084) |
| <Compile Include="System\Transactions\Configuration\ConfigurationStrings.cs" /> | ||
| <Compile Include="System\Transactions\Configuration\DefaultSettingsSection.cs" /> | ||
| <Compile Include="System\Transactions\Configuration\MachineSettingsSection.cs" /> | ||
| <Compile Include="System\Transactions\Trace\TraceRecord.cs" /> |
There was a problem hiding this comment.
e\TraceRecord.cs" / [](start = 46, length = 19)
I am wondering if we can keep these files and "hide" the usage of ETW inside these methods. This might prevent the need for changing all of the scattered locations in the code where the tracing is produced.
There was a problem hiding this comment.
If we reuse the TraceRecord, we may not be able to use [callermethodname]. Also, the code will have one extra layer of function call and not look like other ETW tracing code. This will increase confusing on other developers.
|
🕐 |
|
@dotnet-bot test ci please |
|
LGTM once you fix the merge conflicts |
# Conflicts: # src/System.Transactions/src/project.json
No description provided.