Initialize RequestTelemetry with CorrelationId information #385
Conversation
… and temporarily disable the target framework 4.5.1
…und a timing issue
…target framework upgraded
…stener with latest AspNetCore instrumentation that contains the CorrelationId information. 3) Fix unit tests with .net core
…header only has the standard header but not the new Request-Id header
|
@yantang-msft, |
| "frameworks": { | ||
| "net46": { | ||
| "dependencies": { | ||
| "Microsoft.ApplicationInsights.DependencyCollector": "2.4.0-beta1-build09989", |
There was a problem hiding this comment.
The DependencyCollector will not work if we target "net46", because it pick up the dll from the Net45 folder, which is not compatible with the System.Net.Http that is brought by AspNetCore 2.0
| /// <param name="condition">The condition to meet</param> | ||
| /// <param name="timeoutMs">Timeout in ms</param> | ||
| /// <param name="intervalMs">Sleep interval in ms</param> | ||
| public static void ConditionalTimeout(Func<bool> condition, int timeoutMs = 10000, int intervalMs = 50) |
There was a problem hiding this comment.
Isn't this equivalent to SpinWait.SpinUntil(..) ?
There was a problem hiding this comment.
It's the same... So much better to use this function instead.
| public static void ConditionalTimeout(Func<bool> condition, int timeoutMs = 10000, int intervalMs = 50) | ||
| { | ||
| DateTime timeout = DateTime.Now.AddMilliseconds(timeoutMs); | ||
| while (DateTime.Now < timeout) |
There was a problem hiding this comment.
Never use DateTime.Now for timing loops. It's slow, it has discontinuities and can be adjusted at any point.
DateTime.UtcNow is better but, for this, you should be using System.Diagnostics.Stopwatch.
There was a problem hiding this comment.
Good suggestion! 👍
| return; | ||
| } | ||
| Thread.Sleep(intervalMs); | ||
| } |
There was a problem hiding this comment.
Suggest: throw new TimeoutException();
Or, have this method return a bool (true=>condition met; false=>timeout) and Assert.IsTrue in the caller.
There was a problem hiding this comment.
Changed to SpinUntil
| this.client.Initialize(requestTelemetry); | ||
| requestTelemetry.Start(timestamp); | ||
| requestTelemetry.Start(Stopwatch.GetTimestamp()); |
There was a problem hiding this comment.
It's a shame that we've lost the 'external' timestamp on this method because it allowed us to get very precise timings. I presume it was an intentional change on the ASP.NET Core side (the originator of this HttpRequestIn.Start message).
There was a problem hiding this comment.
It was intentionally lost since Activity exposes StartTimeUtc property.
However there are certain issues with it:
UtcNow precision was improved: dotnet/coreclr#9736 (previously is was ~16ms)
However if AI SDK runs on .NET Desktop, it's not going to get precise timestamp. We'll rely on the local timstamp for now.
There was a problem hiding this comment.
They don't pass the timestamp because the created Activity has StartTimeUtcNow property. But you're right, it feels less accurate. I can create an issue for AspNetCore if you think it's important.
@dnduffy, Here is the change we made that upgraded the reference of AspNetCore to 2.0