AppId is not parsed with DiagnosticSource HTTP hook on net46

[lmolkova]

Problem

DiagnosticSoure HTTP hook sends Begin/End request for outgoing HTTP request.

If request is redirected (and automatic redirects are allowed), there could be multiple End requests for each redirect.
DependecyCollector relies on the EventSource EndRequest to determine when request ends.

According to @nizarq and @xiaomi7732 experiments, EnventSource EndRequest happens before DiagnosticSource Http hook EndRequest. And event source event does not have response object to parse headers from.

So the dependency is tracked without appId from the response headers.

Another problem is that we do not have tests to cover this issue

Solution1: do not support redirects

In case there are no redirects it's simple: there is just one End.
In case there is a redirect, we report only first response: it has wrong duration (only first response duration) and probably no appId.

I wonder, how profiler treats redirects? It should have exactly the same problem

Solution2 (probably a workaround)

We have to wait for the last End from the DiagnosticSource Http hook and track dependency there, not relying on EventSource to do it.

Following code may be used to determine if the End is the last one (see HttpWebRequest code):

        static bool IsLastResponse(HttpWebRequest request, HttpWebResponse response, int redirectNumber)
        {
            if (request.AllowAutoRedirect && 
                IsAutoRedirectStatus(response.StatusCode) &&
                redirectNumber < request.MaximumAutomaticRedirections)
            {
                return false;
            }

            return true;
        }

        static bool IsAutoRedirectStatus(HttpStatusCode responseStatusCode)
        {
            return responseStatusCode == HttpStatusCode.Ambiguous ||   // 300
                   responseStatusCode == HttpStatusCode.Moved ||   // 301
                   responseStatusCode == HttpStatusCode.Redirect ||   // 302
                   responseStatusCode == HttpStatusCode.RedirectMethod ||   // 303
                   responseStatusCode == HttpStatusCode.RedirectKeepVerb;   // 307
        }

The only problem is that we have to count number of redirects to understand where to stop.

Solution3 (probably a long term solution)

We can probably do the trick from solution 2 in DiagnosticSource, however there we must not count redirects manually and we have to use private reflection to get current redirect count from HttpWebRequest, which we should clarify with legals first. It would probably take at least a week just to have an agreement.

We can start with solution1 or solution2 and see if we can eventually move it into DiagnosticSource.

/cc @SergeyKanzhelev

[SergeyKanzhelev]

So does it repro for profiler as well?

How reliable is option 2?

[lmolkova]

After careful consideration, options 2 does not seem correct: if the fix is eventually implemented in DiagnosticSource, DiagnosticSource will fire only one event and AppInsights will ignore it and wait for the next one. So telemetry will not be reported.

So the right approach: do option1 now, and drive option3 in DiagnosticSource for the longer term.

[dnduffy]

Can we tell that we should expect the headers?
Can we tell that it is a redirect?
It seems we should be able to detect and track enough state to know what to do in one of 'N' EndRequest calls.

[lmolkova]

Can we tell that we should expect the headers?

We have no idea if we are calling service has AppInsights or not, so no, we should not expect headers

Can we tell that it is a redirect?
It seems we should be able to detect and track enough state to know what to do in one of 'N' EndRequest calls.

We can, but knowing that it is redirect is not enough:

1. if redirects are not allowed, it is the final response
2. if only 2 redirects are allowed, and it's 3rd response, this is the last one
3. we can count redirects, but eventually we need to do this in DiagSource, and if we implement this logic now in AppInsights, and later fix DiagSource, we will end up with 1 response, but will wait for more.

[cijothomas]

Lets open an issue to investigate profiler case.

"I wonder, how profiler treats redirects? It should have exactly the same problem"

[lmolkova]

@cijothomas
Done #521

[lmolkova]

Fixed with #516 #538