Adding support for HttpHandlerDiagnosticListener. #18101

brahmnes · 2017-04-08T00:36:25Z

This listener compensates for the fact that .NET45/46 doesn't use DiagnosticSource. This solution attempts to recreate that by inserting into the Http stack and generating events.

This HttpHandlerDiagnosticListener is a singleton, and the bulk of its initialization happens when the first subscribe subscribes.
The way this listener injects inself into the Http stack is via reflection. More details are in the code documentation.

This listener compensates for the fact that .NET45/46 doesn't use DiagnosticSource. This solution attempts to recreate that by inserting into the Http stack and generating events. This HttpHandlerDiagnosticListener is a singleton, and the bulk of its initialization happens when the first subscribe subscribes. The way this listener injects inself into the Http stack is via reflection. More details are in the code documentation.

dnfclas · 2017-04-08T00:36:29Z

@brahmnes,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

lmolkova · 2017-04-08T01:19:04Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+            Guid loggingRequestId = Guid.Empty;
+
+            // TODO: Figure what's the right way to detect error conditions here.
+            if (response.StatusCode >= (HttpStatusCode)400)


I don't think Exception event should be sent on >=400 status code.
Consumer can always use response object and status code.

Agree, removed the ExceptionEventName and the status code checking logic.

lmolkova · 2017-04-08T01:31:21Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticListener.cs

@@ -42,6 +42,8 @@ public static IObservable<DiagnosticListener> AllListeners
        {
            get
            {
+                GC.KeepAlive(HttpHandlerDiagnosticListener.s_instance);


you probably want to do it only for net45/net46/netfx

Definitely put a #if NET45 || NET46 || NETFX around it.
Also put a comment indicating its purpose (on Desktop platforms to insure HttpHandlerDiagnosticListener.s_instance has been initialized, which in turn will insure that a DiagnosticListener for the HTTP requests is injected into the desktop framework using reflection).

This hook moved to be in the body of the if (s_allListenerObservable == null) below (it only has to be called one time).

into the framework for HTTP

I am not claiming we support NET45, since I am not going back to verify 4.5. I am just putting NET46 and NETFX on it.

pharring · 2017-04-10T16:53:13Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+using System.Reflection.Emit;
+
+// This HttpHandlerDiagnosticListener class is applicable only for .NET 4.5/4.6, and not for .NET core.
+// If you are making these changes, please test your changes manually via custom test applications.


What does "these changes" in the 2nd sentence refer to?
Also where are the "custom test applications"? Can you provide a link in the comment?

I chatted with Vance and currently the test project only works with .NET core. For testing against .NET 45/46, the best option right now is manual tests.

We should be able to have the tests here. It is only that we don't run the automatically (so you have to run them by hand).

We should be able to have the tests here. It is only that we don't run the automatically (so you have to run them by hand).

Doing this in the test folder will run the tests against netfx (Desktop):

"msbuild /t:rebuildandtest /p:TargetGroup=netfx"

You can also this do (but you'll get other failures right now):

@dotnet-bot test outerloop netfx Windows_NT Debug Queues OuterLoop netfx Windows_NT Debug x64
@dotnet-bot test outerloop netfx Windows_NT Release Queues OuterLoop netfx Windows_NT Release x64

Thanks for the tip. Will add some test in my 3rd iteration.

pharring · 2017-04-10T16:53:50Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+{
+    /// <summary>
+    /// A HttpHandlerDiagnosticListener is a DiagnosticListener for .NET 4.5/4.6 where HttpClient
+    /// doesn't have a DiagnosticListener built-in. This class is not used for .NET Core because


Nit: built in. (two words, not hyphenated)

vancem · 2017-04-10T16:53:59Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        {
+            Initialize();
+            return base.Subscribe(observer, isEnabled);
+        }


You need to add one more overload of Subscribe

public IDisposable Subscribe(IObserver<KeyValuePair<string, object>> observer)

This is not obvious because it is part of the IObservable pattern, but you also want that one to call Initialize().

I also noticed this subtlety. It means I would need to make this Subscribe method on DiagnosticListener virtual as well. Would that cause existing applications that calls Subscribe to fail? I don't know if the IL for virtual vs non-virtual method is different.

The overload above IS virtual (you have to be virtual to override an interface method). It is simply a feature of C# that you don't have to be explicit about it. If you add the method (you don't need override) it will do what we want (which is override anyone calling the interface).

That is actually not true.
I just ran a quick experiment:

using System;
namespace ConsoleApp4
{
public interface IMyInterface
{
void FuncA();
}
public class MyClass1 : IMyInterface
{
public void FuncA()
{
Console.WriteLine("From class 1");
}
}
public class MyClass2 : MyClass1
{
public void FuncA()
{
Console.WriteLine("From class 2");
}
}
class Program
{

static void Main(string[] args) { IMyInterface o = new MyClass2(); o.FuncA(); } }

}

It called MyClass1.FuncA, and not MyClass2.FuncA. If my experiment looks incorrect, let me know.

It is OK to mark DiagnosticListener.Subscribe virtual.

Just a minute. In your example, MyClass2 doesn't implement (some people say "derive from") IMyInterface, so of course it doesn't work. You should instead get a warning from the compiler about hiding the original FuncA (requiring a 'new' if that's what's intended).

I took a look at DiagnosticListener's implementation and I see why it's tempting to make the 1-arg version of Subscribe virtual. But, as as I hinted at above, you don't actually need to: Just implement the IObservable<KeyValuePair<string, object>> on this class.

If we could go back and design DiagnosticListener again, I would propose we introduce a single protected virtual method called SubscribeCore which has the same signature as SubscribeInternal and, for the base class just calls SubscribeInternal. That way, implementations like this have just one method to override. (Actually, we could still do that without breaking any existing code).

First, I don't see any issue with Brahmnes suggestion of making Subscribe(IObserver) virtual. Given that doing this is trivial, this is the solution to beat.

I also toyed with the idea of simply implementing IObservable.Subscribe(Observer) on HttpHandlerDiagnosticListener but it has a subtle bug in that if an instance of HttpHandlerDiagnosticListener is typed as a DiagnosticListener (the common case), and you call Subscribe(IObserver) on it, then you don't get our override (it goes to the nonvirtual method defined on DiagnosticListener).

I don't think adding a protected SubscriberCore is warranted at this time. If we truly believe that people want to override subscribe of DiagnosticListeners, that would be one thing, but frankly that is not at all clear. The alternative (overriding all three overloads), is not really a big deal, and we can always do it later (as you point out).

pharring · 2017-04-10T16:55:07Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+    /// that in .NET 4.5/4.6. HttpHandlerDiagnosticListener has no public constructor, but it has a
+    /// singleton style Initialize method to control its creation.
+    /// 
+    /// To use this, the application just needs to call HttpHandlerDiagnosticListener.Initialize(), and this


As I mentioned elsewhere, an application cannot call Initialize because it's private (and non-static). Also, please use <see cref="..." /> when referring to methods/types/etc.

[Edit: to escape xml doc comment]

I updated the comment to explain how to initialize this object

pharring · 2017-04-10T16:55:43Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+    /// singleton style Initialize method to control its creation.
+    /// 
+    /// To use this, the application just needs to call HttpHandlerDiagnosticListener.Initialize(), and this
+    /// will register itself with the DiagnosticListener's all listeners collection.


Use a cref for DiagnosticListener.AllListeners

vancem · 2017-04-10T16:58:18Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        /// </summary>
+        private void Initialize()
+        {
+            lock (this.initializationLock)


I would just go ahead and use lock(this) for this lock. It avoids another object allocation in the constructor of this DiagnosticListener and generally simplifies the code. We have been allergic to using lock(this) because historically we cared about inprocess hostile code (thus people outside might 'lock' us and force a deadlock) but we really don't care about that here.

As I've mentioned elsewhere, you don't need a lock if you use static initialization.

ok, will change that.

vancem · 2017-04-10T17:00:42Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+                    }
+                    catch (Exception)
+                    {
+                        // If anything went wrong, just no-op


We should go head and do a Write("ReflectionInitializationFailed", new { Exception = e }), so that errors are more noticeable. It cost us next to nothing, and could make debugging significantly easier.

Good idea, added.

pharring · 2017-04-10T17:02:37Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        /// threading issues. If Initialize has been called before, it will not doing anything.
+        /// </summary>
+        private void Initialize()
+        {


I know we've discussed this elsewhere, but I'd like to re-recommend that you make this static and do all the reflection/injection set-up in a static constructor. It doesn't have to be a static constructor on this type - it could be in a nested type, but it would remove the need for initializationLock.

Just make sure that cure is not worse than the disease. Making a little function for each of the s_* fields that is used to initialize them (and thus become part of a nice big .cctor), is fine (because the code is arguably cleaner) But don't do unnatural things just to avoid a lock(this) {} here.

I am following Vance's suggestion of using lock(this), so the extra lock object can disappear. As for the lock(this) statement, since multiple threads can call Subscribe at the same time, I don't see how we can make this completely lock free.

The CLR provides atomic initialization of class constructors. You don't need a lock if you use static initialization. Talk to me offline if you need help.

I tried and couldn't do it cleanly. Basically I would need to add an inner class with the static constructor. This inner class would have the static constructor. However, in the case of a failure, I cannot let any exceptions leaving the static constructor, so I need DiagnosticListener.Initialize() to check on some validity of some fields, to see if it's null (indication that it failed), and write the failure event with Write() if needed. However, to make sure I don't write multiple instances of this event, I need to know if this is the very first time, which puts me back to having a flag.

There may be a pattern to achieve this cleanly, but it's not something I can figure out in a sitting and this is not a critical blocker. Paul, if you have ideas or I am missing something, please feel free to suggest. We can add it in another iteration.

pharring · 2017-04-10T17:04:05Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        /// It also intercepts all HttpWebRequest objects that are about to get removed from
+        /// Connection.m_WriteList as they have completed the request.
+        /// </summary>
+        private class HttpWebRequestArrayList : ArrayList


pharring · 2017-04-10T17:04:17Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        /// intercept each new Connection object being added to ConnectionGroup.m_ConnectionList
+        /// and replace its m_WriteList arraylist field.
+        /// </summary>
+        private class ConnectionArrayList : ArrayList


pharring · 2017-04-10T17:04:26Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        /// intercept each new ConnectionGroup object being added to ServicePoint.m_ConnectionGroupList
+        /// and replace its m_ConnectionList arraylist field.
+        /// </summary>
+        private class ConnectionGroupHashtable : Hashtable


vancem · 2017-04-10T17:14:41Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        internal static HttpHandlerDiagnosticListener s_instance = new HttpHandlerDiagnosticListener();
+
+#region private fields
+        private const string DiagnosticListenerName = "System.Net.Http.Desktop.V4";


Change the .Desktop.V4 suffix to .Net45. This is the correct name for the earliest desktop version that this code supports. We may end up making later versions of this (if we can't do it compatibly).

I actually removed the V4 suffix, so it's just called System.Net.Http.Desktop.
I think this is cleaner and not needing to version rev this. Let me know if you think this is not good enough.

pharring · 2017-04-10T17:14:49Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+                        // which allows us to intercept each new Connection object added under
+                        // this ConnectionGroup.
+                        ArrayList originalArrayList = s_connectionListField.GetValue(value) as ArrayList;
+                        ConnectionArrayList newArrayList = new ConnectionArrayList();


Suggestion: Add a ConnectionArrayList constructor that takes a System.Collections.ICollection parameter and then you can avoid the copy here. (Although you'll need a null check). I would also suggest overriding AddRange so you can make the constructor simpler. i.e.

private sealed class ConnectionArrayList : ArrayList { public ConnectionArrayList(ICollection collection) : base(collection) { } public override void AddRange(ICollection collection) { // Implement in terms of overridden Add } public override int Add(etc... }

I am adding a hash table wrapper and an array list wrapper. The idea is that these wrapper are better with race conditions because they reference the same table/list as the underlying data structures. That change should also remove the need for copying.

pharring · 2017-04-10T17:15:28Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+                    // which allows us to intercept each new HttpWebRequest object added under
+                    // this Connection.
+                    ArrayList originalArrayList = s_writeListField.GetValue(value) as ArrayList;
+                    HttpWebRequestArrayList newArrayList = new HttpWebRequestArrayList();


Similar to my comment on initializing ConnectionArrayList, add a constructor that does the AddRange for you.

Addressed, see above.

CIPop

Please sign the CLA
The change is relying on internals of the HTTP managed stack that are not guaranteed to exist in future versions of .Net Framework.
There are no tests to prove this actually works.

CIPop · 2017-04-10T17:30:49Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+            // The caller should catch all exceptions and swallow.
+
+            // First step: Get all the reflection objects we will ever need.
+            Assembly systemNetHttpAssembly = typeof(ServicePoint).Assembly;


/cc @weshaggard, @davidsh, @karelz

There are several concerns here (aside from the unsigned CLA):

The solution is based on reflection over .Net Framework internals that are not guaranteed to exist in the next versions.

The code is separated from the .Net Framework code which is not public and so a test showing that System.Net changes may affect it is not possible.

There are no tests to verify this even works.

Will add tests in my 3rd iteration.

pharring · 2017-04-10T17:38:48Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        /// intercept each new ServicePoint object being added to ServicePointManager.s_ServicePointTable
+        /// and replace its ConnectionGroupList hashtable field.
+        /// </summary>
+        private class ServicePointHashtable : Hashtable


Added wrapper classes for hashtable and arraylist to address race condition concerns. Renamed the DiagnosticSource to not have the V4 suffix Also many other mini changes per feedback

dnfclas · 2017-04-11T05:16:26Z

@brahmnes, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

pharring · 2017-04-11T14:00:15Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+    /// the application just needs to call <see cref="DiagnosticListener.AllListeners" /> and
+    /// <see cref="DiagnosticListener.AllListenerObservable.Subscribe(IObserver{DiagnosticListener})"/>,
+    /// then in the <see cref="IObserver{DiagnosticListener}.OnNext(DiagnosticListener)"/> method,
+    /// when it seems the System.Net.Http.Desktop source, subscribe to it. This will trigger the


Typo: 'sees' (not 'seems')

pharring · 2017-04-11T14:06:04Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        private class SimpleHashtableWrapper : Hashtable, IEnumerable
+        {
+            protected Hashtable _table;
+            public override int Count


Are you allowed to use expression-bodied members in corefx? If so, these wrapper methods are all one-liners.
i.e.

public override int Count => this._table.Count; public override bool IsReadOnly => this._table.IsReadOnly; etc.

Looks nice. Don't know if I am allowed to use them.

pharring · 2017-04-11T14:10:57Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+            }
+            internal SimpleHashtableWrapper(SerializationInfo info, StreamingContext context) : base(info, context)
+            {
+                this._table = (Hashtable)info.GetValue("ParentTable", typeof(Hashtable));


Nit: Introduce a private const string for "ParentTable"

pharring · 2017-04-11T14:14:30Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+#region private helper classes
+
+        [Serializable]
+        private class SimpleHashtableWrapper : Hashtable, IEnumerable


Would just HashtableWrapper suffice?

Sure, fixed.

pharring · 2017-04-11T14:24:27Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+                            else
+                            {
+                                newTable = new ConnectionGroupHashtable(new Hashtable());
+                            }


Lines 240-246 could be a one-liner:

ConnectionGroupHashtable newTable = new ConnectionGroupHashtable(originalTable ?? new Hashtable());

pharring · 2017-04-11T14:44:47Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        /// </summary>
+        private void Initialize()
+        {
+            lock (this)


Suggest: Use double-checked locking and handle the exception outside the lock. Like this:

private static Exception PrepareReflection() { try { PrepareReflectionObjects(); PerformInjection(); return null; } catch (Exception ex) { return ex; } } private void Initialize() { if (this.initialized) { return; } Exception ex; lock (this) { if (this.initialized) { return; } // This flag makes sure we only do this once. Even if we failed to initialize in an // earlier time, we should not retry because this initialization is not cheap and // the likelihood it will succeed the second time is very small. this.initialized = true; ex = PrepareReflection(); } // If anything went wrong, just no-op. Write an event so at least we can find out. if (ex != null) { this.Write(InitializationFailed, new { Exception = ex }); } }

It is not clear why doing the Write outside the lock is important, and it does complicate the code.

However thinking about it the Write() call is not useful and we should probably just get rid of it. The problem is that we KNOW that this code happen JUST BEFORE the first SUBSCRIBE to this DiagnosticListener. When at that point there are GUARENTEED to be no subscribers, so the Write HAS to do nothing.

Fixing this would require remembering the exception and logging it when the subscribers come along. I don't think this is worth it as the exception was already thrown, so we do have SOME indication that something went wrong, and it is not clear that having the DiagnosticSource tell us is that important (we can add it later if we find it is useful, but frankly this kind of thing is best effort).

Thus I think we should just catch and swallow the exception for now (Brahmnes original code).

I just realize the other simple possibility is reverse the order. It's weird, but less so, and the code is simpler.

public override IDisposable Subscribe(IObserver<KeyValuePair<string, object>> observer, Predicate<string> isEnabled) { IDisposable result = base.Subscribe(observer, isEnabled); try { Initialize(); } catch (Exception ex) { // If anything went wrong, just no-op. Write an event so at least we can find out. this.Write(InitializationFailed, new { Exception = ex }); } return result; }

I think I saw Paul mention this. You should put the try-catch in Initialize (avoid code cloning).

I do like the simple solution to Initialize after Subscribing however.

Made the 3rd DiagnosticListener.Subscribe method virtual and override it Added tests

pharring · 2017-04-11T22:25:53Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+using System.Runtime.Serialization;
+
+// This HttpHandlerDiagnosticListener class is applicable only for .NET 4.6, and not for .NET core.
+// If you are making these changes, please test your changes manually via custom test applications.


This comment is still confusing. What are "these changes"? Where are the "custom test applications"?

Outdated comment. Fixed.

pharring · 2017-04-11T22:31:24Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        public override IDisposable Subscribe(IObserver<KeyValuePair<string, object>> observer, Predicate<string> isEnabled)
+        {
+            IDisposable result = base.Subscribe(observer, isEnabled);
+            try


Can't the try/catch be done inside Initialize? Reduce code duplication.

vancem · 2017-04-11T23:32:04Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+        {
+        }
+
+        private void RaiseRequestEvent(HttpWebRequest request)


I would like to understand why we are not using activities and trying to make this look as much like the .NET Core case as possible. Same goes for the timestamp (We probably need to a 'Activity.UtcNow method that on .NET Core just rebounds to DateTime.UtcNow but on V4.X uses Stopwatch to make high res UtcNow (it is now hard, I can give you code if you like).

Don't we want be extracting and inserting IDs/Baggage (again like .NET Core?)

@lmolkova

I can speak for this. :)
Since this is not a long term solution, we are taking the approach of getting just the hooks in place here and leaving the activity logic in the caller. There are two reasons:

For a single HttpWebRequest, it actually triggers multiple send and receive events. An example is HTTP redirection. DiagnosticSource doesn't have full information to deduplicate these events. Application Insights however, has enough states to handle duplicates and log only one AI event.

Application Insights has a path handling cases where user uses profiler, so it handles all the activity stuff already. It's far easier to share that code path with this new path.

In the long term, we can look at adding activity logic here. However, I kind of like to first see how well this change goes.

Given this all is 'hack' code that in theory goes away, and is 'agile (it is relatively easy to update this and get it deployed), I guess I could live with this. Still I think we overweighting the 'App Insights' scenario here. Pretty much ANYONE using this diagnostic source (e.g. Glimpse etc) who cares about HTTP is likely to want the ID passing logic, so that argues that we should do it once for everyone here. Also if we update this logic in the HttpClient (e.g. add interoperability with OpenTracing features (exactly what header name is used ...)), we could simply add that logic here as well and have a complete feature that works on .NET Core AND .NET Desktop. If we delegate to the client of this hook, any updates to HttpClient need coordination with at least the AI team and probably more. Feels like a mess.

For things like ID and Baggage passing, de-duping is easy (if it is there already do nothing), so it seems like we should at least be doing that.

Comments?

I agree that having the same logic here and in HttpClient is a good thing. De-duplication could be based on the presence of headers in the request.
Events names then should be renamed to be consistent with HttpClient, the whole IsEnabled flow also goes here.

The problem is that the change to AppInsights (that consumes the hook) was done in the last minute before the code freeze and now it is probably too late to do any changes there.

I see following options:
1. Do the right thing here, in DiagnosticSource, make it fully repeat HttpClient instrumentation on corefx: create activity and inject headers.
It requires some minor changes in AppInsights @brahmnes, we need to check if AI team is fine allowing us to do this as a bugfix. If they are fine, I can finish this change and fix AI.

If we don't do it now, we either

leave current behavior (no activities, consumer injects headers) forever

OR have to deal with events deprecation later (e.g. in the next major DiagnosticSource version)

2. Do not have this hook in DiagnosticSource for now, keep it in AppInsights (it is copy-pasted for testing). Add the hook in the next major release of DiagnosticSource, make some future AppInsights release compatible with it and remove hook from AppInsights.
However there is an agreement with AI team that the the hook will be moved to DiagnosticSource.

3. Leave current behavior, but as i mentioned before we either have to live with this forever or deal with deprecation problems later.

I strongly prefer option 1 as long as AppInsights team agree on taking fix for this release.

Otherwise, option 2 is not that bad: only AI knows how to properly use the hook and it is custom for it, so it lives in AI codebase.

@brahmnes could you discuss this options with AI team?

I think option 1 is fine. We need to make a change to remove the temporary diagnosticlistener in there anyways, we can bundle this with that change. People are usually comfortable if you give good explanations and it's code deletion and not code addition.
Regarding Vance's comment about it being overweighted on Application Insights, I don't think that is true. Outside of correlation ID support, which is the motivation we have, the feature is merely notifying you when HttpWebRequest is about to send and receive. I think it can stand on its own.
Liudmila can look at how to add activity support. Nevertheless, I think we should get this change in first, and then make more changes on top of it.

@vancem
Would you be ok with the following plan?

Merge this change (unless there are other open issues, except activity support)

I will submit another PR with Activity sometime soon (this or next week) that will add activities and header injection.

I am OK with @lmolkova s plan.

thanks! Could you please merge it?

…ght files. Fixed issues in the test code. Removed the serialization attribute for HashtableWrapper because .NET didn't allow it at runtime due to security reasons. Fixed a few issues discovered in product code.

pharring · 2017-04-12T03:14:41Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+using System.Reflection.Emit;
+using System.Runtime.Serialization;
+
+// This HttpHandlerDiagnosticListener class is applicable only for .NET 4.6, and not for .NET core.


How about:

#if !NET46 #error This is for .NET 4.6 only. #endif

The csproj file already conditionally include this file, so the #error is probably redundant.
The truth is this will be needed for .NET 4.7 since .NET 4.7 has released. I also changed the comment to say .NET 4.6 and above. In the future, when we have a better solution implemented natively in HttpClient, we should revisit this component and change.

pharring · 2017-04-12T03:19:43Z

src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

+            {
+            }
+
+            private const string ParentTableSerializationField = "ParentTable";


Is this actually used now? Looks like serialization isn't implemented.

Argh, I missed this. Good catch.
Fixed now, and removed both this const and the OnDeserialization method.

Adding support for HttpHandlerDiagnosticListener. Commit migrated from dotnet/corefx@7aac98a

dnfclas added the cla-required label Apr 8, 2017

lmolkova reviewed Apr 8, 2017

View reviewed changes

karelz added the area-System.Diagnostics.Tracing label Apr 8, 2017

karelz assigned brahmnes, brianrob, davidsh, vancem and CIPop Apr 8, 2017

karelz requested review from davidsh and CIPop April 8, 2017 05:57

karelz added area-System.Net.Http and removed area-System.Diagnostics.Tracing labels Apr 8, 2017

brahmnes mentioned this pull request Apr 10, 2017

Adding support to inject headers and intercept Http outgoing requests microsoft/ApplicationInsights-dotnet-server#429

Closed

pharring reviewed Apr 10, 2017

View reviewed changes

vancem reviewed Apr 10, 2017

View reviewed changes

pharring reviewed Apr 10, 2017

View reviewed changes

vancem reviewed Apr 10, 2017

View reviewed changes

pharring reviewed Apr 10, 2017

View reviewed changes

vancem reviewed Apr 10, 2017

View reviewed changes

pharring reviewed Apr 10, 2017

View reviewed changes

CIPop reviewed Apr 10, 2017

View reviewed changes

pharring reviewed Apr 10, 2017

View reviewed changes

davidsh removed their assignment Apr 10, 2017

Addressed PR feedback.

9f7dfb9

Added wrapper classes for hashtable and arraylist to address race condition concerns. Renamed the DiagnosticSource to not have the V4 suffix Also many other mini changes per feedback

dnfclas added cla-signed and removed cla-required labels Apr 11, 2017

pharring reviewed Apr 11, 2017

View reviewed changes

Incorporated PR feedback

e098f8e

Made the 3rd DiagnosticListener.Subscribe method virtual and override it Added tests

pharring reviewed Apr 11, 2017

View reviewed changes

Incorporated more PR feedback

e8809c9

vancem reviewed Apr 11, 2017

View reviewed changes

Fixing the test project to build only for netfx, and only with the ri…

abc16ec

…ght files. Fixed issues in the test code. Removed the serialization attribute for HashtableWrapper because .NET didn't allow it at runtime due to security reasons. Fixed a few issues discovered in product code.

pharring reviewed Apr 12, 2017

View reviewed changes

Addressing more PR feedback

85ac27a

vancem merged commit 7aac98a into dotnet:master Apr 12, 2017

karelz modified the milestone: 2.0.0 Apr 15, 2017

lmolkova mentioned this pull request Apr 17, 2017

Start Activity and inject headers in HttpHandlerDiagnosticListener on net46 #18502

Merged

This was referenced May 1, 2017

HttpHandlerDiagnosticListener: Write response event once when request is redirected #19195

Merged

Enable HttpHandlerDiagnosticListener on .NET45 #19262

Merged

macrogreg pushed a commit to open-telemetry/opentelemetry-dotnet-instrumentation that referenced this pull request Sep 24, 2020

Merge pull request dotnet/corefx#18101 from brahmnes/master

5f798db

Adding support for HttpHandlerDiagnosticListener. Commit migrated from dotnet/corefx@7aac98a

ericstj mentioned this pull request Jan 4, 2021

Adding support for W3C Baggage header dotnet/runtime#45839

Closed

Adding support for HttpHandlerDiagnosticListener. #18101

Adding support for HttpHandlerDiagnosticListener. #18101

Uh oh!

Conversation

brahmnes commented Apr 8, 2017

Uh oh!

dnfclas commented Apr 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brahmnes Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brahmnes Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vancem Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pharring Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vancem Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pharring Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brahmnes Apr 10, 2017 •

edited

Loading

brahmnes Apr 10, 2017 •

edited

Loading

vancem Apr 10, 2017 •

edited

Loading

pharring Apr 11, 2017 •

edited

Loading

vancem Apr 11, 2017 •

edited

Loading

pharring Apr 10, 2017 •

edited

Loading