[Proposal]Implement ITelemetryInitializer to handle UseUniqueServiceUrl

[mkosieradzki]

ServiceFabric AspNetCore middleware has an option ServiceFabricIntegrationOptions.UseUniqueServiceUrl. This option generates service-specific unique urls - which makes ServiceFabric telemetry difficult to correlate.

See microsoft/service-fabric-aspnetcore#18 .

IMO it would be really useful to have a telemetry initializer that strips autogenerated urls.

I can create a PR for this.

[nizarq]

Sorry for the delayed response.

When you say it makes ServiceFabric telemetry difficult to correlate, what correlation are you talking about?
Are you talking about he following from microsoft/ApplicationInsights-aspnetcore#463:

Application Insights for ASP.NET Core are printing following events:
Microsoft.ApplicationInsights.Message, Microsoft.AspNetCore.Hosting.Internal.WebHost, http://localhost:1234, Connection id "0HL552DLRQAR8" completed keep alive response.

and then:
Microsoft.ApplicationInsights.Request, with url: https://mydomain.com

What are the sources of the above telemetry? It seems like request telemtry is gathered by the request tracking module in the AI sdk, but the message might be coming from Windows Azure Diagnostics agent running on the SF node that forwards ETW events to AI. If that is so, wouldn't the fix be make whatever component is generating the url with http:localhost1234, report the original url at which point we don't have to deal with the autogenerated guid at all?

How do you propose to fix this? Is it easy to tell given a url, which part is auto generated?

We would love to take a PR should the result of this thread be - we need a change.

[mkosieradzki]

@nizarq I apologize for making this confusing. I will try to clear this up:
Let's ignore the issue with Microsoft.ApplicationInsights.Message and let's focus on Microsoft.ApplicationInsights.Request telemetry which is generated by https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/801cf8a79ad34588611f133bde9c7c59a57dc783/src/Microsoft.ApplicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs#L265-L269

It is based on HttpContext. If one is using ServiceFabricIntegrationOptions.UseUniqueServiceUrl the URL will look not very nice http://localhost:1234/{generated unique url}/mypath or http://mydomain/{generated unique ur}/mypath depending on whether developer is using UseForwardedHeaders middleware.

I use following code to conditionally add my telemetry initializer.

if (removeUniqueUriPart)
{
    services.AddSingleton<ITelemetryInitializer>(new RemoveUniqueUriPartInitializer());
}

and below is my initializer that works:

    sealed class RemoveUniqueUriPartInitializer : ITelemetryInitializer
    {
        private const int SegmentsToSkip = 3;

        public void Initialize(ITelemetry telemetry)
        {
            var requestTelemetry = telemetry as RequestTelemetry;
            if (requestTelemetry != null)
            {
                var builder = new UriBuilder(requestTelemetry.Url);

                var originalPath = builder.Path;
                var index = 0;
                for (int i = 0; i < SegmentsToSkip; i++)
                {
                    index = originalPath.IndexOf('/', index + 1);
                    if (index < 0)
                        return;
                }

                var newPath = originalPath.Substring(index);

                builder.Path = newPath;
                requestTelemetry.Url = builder.Uri;
                requestTelemetry.Name = requestTelemetry.Name.Replace(originalPath, newPath);
            }
        }
    }

It's really trivial and it just removes first 3 segments. BUT I can make it much better if Microsoft.ServiceFabric.AspNetCore could be isolated into a separate package (today it is inside Microsoft.ServiceFabric.AspNetCore.Kestrel and Microsoft.ServiceFabric.AspNetCore.WebListener).

I could then take AspNetCoreCommunicationListener and extract its url suffix like here:
https://github.com/Azure/service-fabric-aspnetcore/blob/develop/src/Microsoft.ServiceFabric.AspNetCore/WebHostBuilderServiceFabricExtension.cs#L52

BTW. By correlation I mean correlation by telemetry display name or url in Application Insights UI.

[nizarq]

• @vturecek to inject service fabric expertise into the discussion.

@mkosieradzki - Is ServiceFabricIntegrationOptions.UseUniqueServiceUrl only an asp.net core option. The problem it tries to solve - that of a different service using the same port after the original service had to be migrated - is not an asp.net core only possibility. Is there an equivalent for .net framework that we need to worry about as well?

Also is the reverse proxy involved? Are you running the reverse proxy at http://mydomain? I imagine reverse proxy or no reverse proxy (direct calls), you would have to deal with this issue - correct?

Removing the first three segments seems a little too arbitrary for the following reasons:

1. You are not only removing the arbitrary {generated unique url} part but also the segments preceeding it which may contain important information. With no reverse proxy invovled, imagine two services Service1 and Service2 both accept a path /api/count and are running on different ports. Wouldn't removing the starting segment mean now you cannot distinguish between calls to Service1 and Service2.
2. When using the UseUniqueServiceUrl option, does SF append the guid before the service name or after the service name but before the path. If later, the service name itself span multiple segments (which is indeed very common).

[mkosieradzki]

1. Reverse proxy is not involved here. Reverse proxy takes part only if someone uses UseForwardedHeaders - but this specific middleware does not play with paths (only Host and Scheme).
2. Yes I am removing arbitrary fragments in my prototype but I will switch to remove the proper prefix returned by the listener.
3. ASP.NET Core works on both .NET Core and .NET Framework so we only need to care about ASP.NET Core. Legacy ASP.NET is not working in microservices environment at all (maybe except OWIN, but it is legacy as well and directly upgradable to ASP.NET Core).
4. There should be no preceding segments as stated here:
   https://github.com/Azure/service-fabric-aspnetcore/blob/develop/src/Microsoft.ServiceFabric.AspNetCore/ServiceFabricMiddleware.cs#L73-L73
   I think we should follow exactly this code: https://github.com/Azure/service-fabric-aspnetcore/blob/develop/src/Microsoft.ServiceFabric.AspNetCore/ServiceFabricMiddleware.cs#L82-L92

If @vturecek will agree to isolate Microsoft.ServiceFabric.AspNetCore into a separate package (microsoft/service-fabric-aspnetcore#20) we can use the listener to configure our middleware and keep everything consistent. In the meantime I will rework my middleware prototype to use UrlPrefix.

[vturecek]

The middleware sets Path and PathBase accordingly on HttpRequest. Path contains just your API path, e.g., /api/values, while PathBase has the unique URL path segments, e.g., /partitionId/replicaId/GUID.

So rather than parse out the unique URL segments, can you just grab the value of Path?

[mkosieradzki]

@vturecek I have explained why I can't do this in microsoft/service-fabric-aspnetcore#18 . Basically middleware recovers previous Path and PathBase values so during OnStarting or OnCompleted when telemetry is being generated PathBase is set to / and Path is set to full path.

I was even proposing to alter this behavior for simplicity, but we agreed it's better to keep it as is for sake of consistency with other middlewares that also recover Path and PathBase.

[vturecek]

@mkosieradzki Path and PathBase get reset for middleware that runs before the SF middleware, but any middleware that runs after will have the unique path in PathBase. So couldn't you just run your middle after the SF middleware?

[mkosieradzki]

@vturecek Not really, because:

1. this middleware is registering Application Insights ITelemetryInitializer on which I have no control when it is run (it is run when request is completed so it has measurements of request time etc).
2. Any OnStarting or OnCompleted event is run on the reverted Path and PathBase.

[vturecek]

@nizarq I see what @mkosieradzki is after now, this might be something we can handle in this package.
@mkosieradzki we can consider splitting the SF asp.net core packages if there's enough reason to take that on but I'd like to see if we can solve this particular problem for everyone.

[nizarq]

Yeah this makes more sense now.
My only remaining concern is figuring out packaging.

Today - the nuget packages (Microsoft.ApplicationInsights.ServiceFabric and Microsoft.ApplicationInsights.ServiceFabric.Native) this repo ships are agnostic to the kind of application.
Microsoft.ApplicationInsights.ServiceFabric.Native deals with all reliable services (asp.net core, .net stateful or stateless service with or without a service endpoint). Whereas Microsoft.ApplicationInsights.ServiceFabric deals with anything and everything including guest executables and containerized lift and shift applications.

The TelemetryInitializer proposed here really only makes sense for 'asp.net core' applications. We should NOT ship it in one of the existing packages. Perhaps create an additional Microsoft.ApplciationInsights.ServiceFabric.AspNetCore?

[mkosieradzki]

So I think we need two more packages:
Microsoft.ApplicationInsights.SericeFabric.AspNetCore and Microsoft.ServiceFabric.AspNetCore.Abstractions. I pretty much don't see a way around.

[nizarq]

@mkosieradzki - I don't mind an extra package Microsoft.ApplicationInsights.ServiceFabric.AspNetCore. Please close on the discussion on AspNetCore.Abstractions and feel free to submit a PR with the new TelemetryInitializer and the associated nuget package project.

[mkosieradzki]

@nizarq thanks.

When I get the green light on AspNetCore.Abstractions I will try to create a PR :).

[nizarq]

Hi @mkosieradzki - I noticed that the abstractions package is now available. Is this issue still something of interest to you? Do you plan to submit a PR or should we close it due to lack of interest?

[mkosieradzki]

Hi @nizarq I will try to revisit this issue next month. I hope I can get ApplicationInsights-ServiceFabric working with .NET Standard...

[nizarq]

I have a change that will allow you to do that. I'll merge it soon.