Overload StartOperation to specify operation Id and parent Id #430
Conversation
|
Hi @pharring, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
|
Would you mind to make another overload that takes an entire |
Certainly. Done. |
| { | ||
| var op = this.telemetryClient.StartOperation<RequestTelemetry>("request"); | ||
| var id1 = Thread.CurrentThread.ManagedThreadId; | ||
| int id2 = 0; | ||
| this.telemetryClient.TrackTrace("trace1"); | ||
|
|
||
| HttpWebRequest request = WebRequest.Create(new Uri("http://bing.com")) as HttpWebRequest; | ||
| var result = request.BeginGetResponse( | ||
| var result = Task.Delay(millisecondsDelay: 50).AsAsyncResult( |
There was a problem hiding this comment.
FYI, this test failed for me because I was working in a place with no WiFi. Hence the change to Task.Delay.
There was a problem hiding this comment.
There is an additional Thread.Sleep(100); call in this method that seems questionable.
I also wonder if the test method itself should be attributed for timeout so that the while (!result.IsCompleted) loop doesn't ever accidentally become an infinite loop.
| } | ||
|
|
||
| var operation = new AsyncLocalBasedOperationHolder<T>(telemetryClient, operationTelemetry); | ||
| operationTelemetry.Start(); |
There was a problem hiding this comment.
I wonder if these two lines (82,83) should be switched (I kept them like this to preserve existing behavior, but I don't think it matters)
- To get the timestamp as close as possible to the top of the method.
- To put the setting of
operation.ParentContextnext to the constructor.
What do you think, @SergeyKanzhelev ?
There was a problem hiding this comment.
I agree - ordering is not logical. If calculating what's inside - Start should be the very last operation. If measuring total - it should be first. Not a big deal, thanks for noticing. You can switch the order
| /// <param name="operationId">Operation ID to set in the new operation.</param> | ||
| /// <param name="parentOperationId">Optional parent operation ID to set in the new operation.</param> | ||
| /// <returns>Operation item object with a new telemetry item having current start time and timestamp.</returns> | ||
| public static IOperationHolder<T> StartOperation<T>(this TelemetryClient telemetryClient, string operationName, string operationId, string parentOperationId = null) where T : OperationTelemetry, new() |
There was a problem hiding this comment.
One of the problem with our Sampling + Correlation is that we calculate hash on Context.User.Id (if set), than Context.Operaiton.Id (if set), than toss a coin. The idea was to either include or exclude telemetry for the user.
Passing parent and root solves the problem in case Context.User.Id is not set. However if it was set for the request - "childrens" wouldn't get it.
So we may need to have override with the userId as well. Let me chat with PMs. This sampling by userId not quite working with the correlation story we are implementing today...
There was a problem hiding this comment.
Presumably, if an app wants to flow Context.User.Id from parent to child, then it would need to install an initializer like UserTelemetryInitializer from the ASP.NET repo. That seems reasonable to me.
|
I'll merge later today. Giving chance @dnduffy and @cijothomas to review. |
| } | ||
|
|
||
| /// <summary> | ||
| /// Ensure that context being propagated via async/await. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void ContextPropogatesThruAsyncAwait() | ||
| public void ContextPropagatesThruAsyncAwait() |
There was a problem hiding this comment.
nitpick: There is an additional (pre-existing) spelling mistake, "Thru" should be "Through".
| { | ||
| using (var operation1 = this.telemetryClient.StartOperation<RequestTelemetry>("OuterRequest")) | ||
| { | ||
| using (var operation2 = this.telemetryClient.StartOperation<DependencyTelemetry>("DependentCall")) |
There was a problem hiding this comment.
nitpick: I have a strong personal dislike of using var instead of known types but in these usings in particular it really isn't clear what the type returned by StartOperation is.
There was a problem hiding this comment.
Fixed (although I did not replace var in tests that were already in place before this change)
Fixes #163