Detect broken Activity stack and gracefully end the loop #23
Conversation
lmolkova
changed the title
|
please review. |
| { | ||
| // silently stop all child activities before activity | ||
| while (Activity.Current != activity && Activity.Current != null) | ||
| int iteration = 0; | ||
| while (currentActivity != activity) |
There was a problem hiding this comment.
we may need a similar logic in recent PR with the tags reading in base SDK
There was a problem hiding this comment.
in the base SDK we don't walk up the stack of activities to find ours. If current does not match current operation, we just fail. We can do that for sure, I'll create an issue
There was a problem hiding this comment.
I mean for the recent PR where tags are read from all parents
There was a problem hiding this comment.
If you mean this one: microsoft/ApplicationInsights-dotnet#736, it is for initializing/tracking telemetry before Activity is stopped. Basically, it allows customers to use tags themselves with a custom initializer.
For parent tags reading, walking up the stack is not a problem, as Parent for 'broken' Activity is correct, it's just Stop() that never updates Current to Parent.
There was a problem hiding this comment.
I was confused. I though to fix this one: microsoft/ApplicationInsights-dotnet#562 we will walk up the parents. When we will be implementing this we need to have the same protection there
There was a problem hiding this comment.
There is no problem with parent walking, i.e.
var activity = Activity.Current;
while (activity != null) {
// do something
activity = activity.Parent;
}
is not affected by this Stop issue.
It might be a hypothetical problem if we'll ever have a cycle in the parents stack.
I'll update the Tags issue and mention it.
| WriteEvent(6, id, name); | ||
| } | ||
|
|
||
| [Event(7, Message = "Activity stack is too deep, Current Id: '{0}', Name: '{1}'", Level = EventLevel.Error)] |
There was a problem hiding this comment.
It may be a good idea to subscribe on these in base SDK so we have them reported as SDK errors.
| <Reference Include="System.Diagnostics.DiagnosticSource, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL"> | ||
| <HintPath>..\..\packages\System.Diagnostics.DiagnosticSource.4.4.0\lib\net45\System.Diagnostics.DiagnosticSource.dll</HintPath> | ||
| <Reference Include="System.Diagnostics.DiagnosticSource, Version=4.0.2.1, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL"> | ||
| <HintPath>..\..\packages\System.Diagnostics.DiagnosticSource.4.4.1\lib\net45\System.Diagnostics.DiagnosticSource.dll</HintPath> |
There was a problem hiding this comment.
is this reference upgrade necessary? I worry it may break something or introduce some unnecessary binding redirects.
There was a problem hiding this comment.
you are right, it is not necessary
Fix #22