Adding support for HttpHandlerDiagnosticListener.

[brahmnes]

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]

@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]

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

[brahmnes]

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

[lmolkova]

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

[brahmnes]

Agree

[vancem]

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

[brahmnes]

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]

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?

[brahmnes]

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.

[vancem]

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).

[davidsh]

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"

[davidsh]

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

[brahmnes]

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

[pharring]

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

[brahmnes]

Fixed

[vancem]

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().

[brahmnes]

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.

[vancem]

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).

[brahmnes]

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.

[vancem]

It is OK to mark DiagnosticListener.Subscribe virtual.

[brahmnes]

Fixed

[pharring]

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).

[pharring]

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).

[vancem]

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]

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]

[brahmnes]

I updated the comment to explain how to initialize this object

[pharring]

Use a cref for DiagnosticListener.AllListeners

[brahmnes]

Fixed

[vancem]

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.

[pharring]

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

[brahmnes]

ok, will change that.

[vancem]

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.

[brahmnes]

Good idea, added.

[pharring]

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.

[vancem]

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.

[brahmnes]

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.

[pharring]

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.

[brahmnes]

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]

OK

[pharring]

sealed?

[brahmnes]

Fixed

[pharring]

sealed?

[brahmnes]

Fixed

[pharring]

sealed?

[brahmnes]

Fixed

[vancem]

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).

[brahmnes]

Fixed

[brahmnes]

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]

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...

}

[brahmnes]

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]

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

[brahmnes]

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]

/cc @weshaggard, @davidsh, @karelz

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

1. The solution is based on reflection over .Net Framework internals that are not guaranteed to exist in the next versions.
2. 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.
3. There are no tests to verify this even works.

[brahmnes]

Will add tests in my 3rd iteration.

[pharring]

sealed?

[brahmnes]

Fixed

[dnfclas]

@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]

Typo: 'sees' (not 'seems')

[brahmnes]

Fixed

[pharring]

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.

[brahmnes]

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

[pharring]

Nit: Introduce a private const string for "ParentTable"

[brahmnes]

Fixed

[pharring]

Would just HashtableWrapper suffice?

[brahmnes]

Sure, fixed.

[pharring]

Lines 240-246 could be a one-liner:

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

[brahmnes]

Fixed

[pharring]

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 });
    }
}

[vancem]

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).

[brahmnes]

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;
}

[vancem]

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.

[brahmnes]

Fixed

[pharring]

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

[brahmnes]

Outdated comment. Fixed.

[pharring]

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

[brahmnes]

Fixed

[vancem]

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

[brahmnes]

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:

1. 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.
2. 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.

[vancem]

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?

[lmolkova]

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?

[brahmnes]

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.

[lmolkova]

@vancem
Would you be ok with the following plan?

1. Merge this change (unless there are other open issues, except activity support)
2. I will submit another PR with Activity sometime soon (this or next week) that will add activities and header injection.

[vancem]

I am OK with @lmolkova s plan.

[lmolkova]

thanks! Could you please merge it?

[pharring]

How about:

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

[brahmnes]

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]

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

[brahmnes]

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