-
Notifications
You must be signed in to change notification settings - Fork 123
Requests with the url http:/// causes the exception #278
Comments
Thanks @SergeyKanzhelev I have packaged an ASP.NET Core application in a Docker image and use the Application Insights .NET Core SDK. When I run the container under Marathon (on DCOS), the healthcheck URI sometimes shows in the logs as http:///. Unfortunately App Insights request pipeline throws an exception trying to retrieve the host name for these requests. |
I thought I'd saved the exception details, but can't find it. I'll recreate it if I get chance, but this is the line that was throwing IIRC: https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/bde2dcaf60ccbaf9b759597d9482ef4efa774fae/src/Microsoft.ApplicationInsights.AspNetCore/Extensions/HttpRequestExtensions.cs#L31 |
Do you only get |
I set up the healthcheck in marathon to hit `/?healthcheck' (i.e. the root of the site with a healthcheck query string parameter to make the health checks easy to identify. In the logs I see
If I disable Application Insights and redeploy then the app runs fine (i.e. ASP.NET Core is happy with the empty hostname):
|
@Eilon, have you ever seen the empty hostname in URL? BTW, is there a way to get RawUrl from the request instead of constructing it like this: https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/master/src/Microsoft.ApplicationInsights.AspNetCore/Extensions/HttpRequestExtensions.cs#L17? |
@Tratcher would know best. This does seem very odd. |
With System.Uri, if I try
|
Can you provide a full trace of the request? E.g. Fiddler/wireshark/etc.? HTTP/1.0 did not require a Host header, that was a new requirement in HTTP/1.1. The client had a host in their original URL, but they did not transmit it to the server. Kestrel does not require a Host header on any version. You can use GetEncodedUrl https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http.Extensions/UriHelper.cs#L166 to build the url correctly for most scenarios. Your version leaves out PathBase and incorrectly encodes Path https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/master/src/Microsoft.ApplicationInsights.AspNetCore/Extensions/HttpRequestExtensions.cs#L42. However GetEncodedUrl doesn't handle the missing Host scenario any better. When the Host is missing you must either provide a default or generate a relative uri (e.g. only the path and query). The RawUrl is a misconception, there is no full raw url transmitted in an HTTP request, only pieces that must be assembled. |
I can't easily get the trace - I'm running this in Container Service under DCOS and Marathon. Marathon is executing the healthchecks, and the The only reason I tested parsing They're working with In the absence of fiddler capture, is there any extra logging I can enable for ASP.NET Core that would help? |
You can also enable Kestrel's connection level logging: |
Ok, I turned on the Kestrel logging and got the following output for the request without a hostname:
I do also see requests with the hostname:
|
Ok, so it is a 1.0 request with no host header. @SergeyKanzhelev it looks like that method is only used here: https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/56dc5297134406d6b028076aa5b3582311412e74/src/Microsoft.ApplicationInsights.AspNetCore/RequestTrackingMiddleware.cs#L56 for logging purposes. In this case throwing is likely overkill, you should be able to provide defaults for any unknown values. e.g. "http://unspecified/path?query" |
Looks like this still might be a problem in the new implementation in 2.0.0: https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/develop/src/Microsoft.ApplicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs#L116 |
Same problem here - I see it in the logs but do not know if there is any real problem coming out of it (will it throw to user level?) |
The issue I hit was that the healthcheck in DC/OS made the request, got a bad response and decided that the container instance was unhealthy and tore it down |
I see in Application Insights Log (running on Azure, ASP.NET Core on 4.6.2 FULL .Net Framework):
|
Any update on this? A lot of container schedulers seem to do healthchecks without sending in a host header. |
If I read all this correctly then the following is true:
There are two main workarounds:
From an ASP.NET perspective, we could add a configuration option that lets you specify a string to use in place of the host when one isn't required. That would just add another option to the workarounds. But presumably we should be having GetUri not throw. So that leads to this question: @SergeyKanzhelev presumably we need a valid URL for RequestTelemetry.URL. But is it used for more than just information to the user? App Insights could possibly leave it out or do something like use |
Application Insights does not do a lot of parsing of URL on backend. But it is enforces in C#. |
Looking at the code, AppInsights checks whether the host is set here, but that method returns a |
UNKNOWN-HOST |
Fixed with #677 |
Reported by @stuartleeks: Trying to extract the host address from the url 'http:///' causes the exception.
The text was updated successfully, but these errors were encountered: