Application Insights integration : ASP.Net Core review

[daveta]

I'd like to get some eyes on initial commit for ASP.Net Core.

This contains a version of TelemetryWaterfallDialog which will most likely be dumped in some form or fashion, but functionally it performs what we need.

Some possibilities for changes in this PR:

• We could remove BotConfig from AddBotApplicationInsights() and rely that it's been added as a singleton.
• We could move IBotTelemetryClient into BotBuilder (and decouple the Application Insights EventTelemetry so we don't have any dependency on Application Insights, and someone could replace IBotTelemetryClient).
• We could remove the appsettings.json InstrumentationKey validation check in UseBotApplicationInsights(). I've just made that mistake too many times and appreciate the check.

If someone with a Bot that wants to use App Insights, it would look like:

// In Startup.cs
public void ConfigureServices(IServiceCollection services)
  ...
   // Add Application Insights Telemetry components so userid/sessionid
   // are in terms of Bot identifiers.
   // Specifically the Application Insights Telemetry Initializer and
   // IBotTelemetryClient.
   services.AddBotApplicationInsights(botConfig);
   ...
   
public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory)
   ...
   // Add Application Insights Telemetry components so userid/sessionid
   // are in terms of Bot identifiers.  
   // Specifically the ASP.Net Middleware component.
   app.UseBotApplicationInsights()
  ...

[coveralls]

Pull Request Test Coverage Report for Build 44013

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 4 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+1.0%) to 54.136%

---

Files with Coverage Reduction	New Missed Lines	%
/libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/ServiceCollectionExtensions.cs	4	94.87%

Totals
Change from base Build 43927:	1.0%
Covered Lines:	5144
Relevant Lines:	9502

---

💛 - Coveralls

[drub0y]

This includes details on how the current implementation should be changed at bare minimum.

As I said in #1179 tho, I would suggest that the current implementation being an ASP.NET middleware is a flaw in and of itself and should be done as a piece of Bot Middleware instead. It would take more than just some text in a PR to explain how that should be done though, so either I can make the change myself and PR it in or we can have a discussion around it after everyone is back from the holidays.

[drub0y]

So this middleware is going to get in the middle of each request. For all POSTs it's going to effectively kick in the logic below which forces buffering and attempts to read the whole thing into memory as a string (ReadToEnd) and then tries to parse it into a JSON object which, it might not even be JSON.

What if the request is actually a large file upload going to an MVC controller that's co-hosted with the bot? 😕

[drub0y]

NOTE: If it's not obvious, that's a rhetorical question; just trying to call attention to the flaw with the implementation.

[drub0y]

⚠️ If you were to keep this implementation (which I don't suggest you do)... ⚠️

There's no need to bring IMemoryCache cache into the mix here. You have access to the HttpContext and you could just stuff the value into the Items collection using a special key* and that will flow with the context everywhere, which means the TelemetryBotIdInitializer could pull it from there using the same special key.

• Don't use the TraceIdentifier, throw a internal static readonly string field on TelemetryBotIdInitializer and use a value like "__MSBOTFRAMEWORK_TELEMTRYBOTIDINITIALIZER_BODY_JSON_STRING"

[drub0y]

⚠️ If you were to keep this implementation (which I don't suggest you do)... ⚠️

That's a really tiny read buffer size that's gonna cause a lot of extra reads. Typically you'd want to go at least 4k (4096).

[drub0y]

⚠️ If you were to keep this implementation (which I don't suggest you do)... ⚠️

Again, ditch IMemoryCache, but... this should have told us something was wrong with the implementation right away. How can we arbitrarily choose any amount of time like this? What if the bot calls some services and takes > 3s to respond? I mean, the code was defensive about it possibly not being there down the line, sure, but we were ok with just not logging the telemetry in this case?

[drub0y]

⚠️ If you were to keep this implementation (which I don't suggest you do)... ⚠️

Ok, so... I don't see the value in parsing the JSON here at all yet. You only need to understand the JSON in the TelemetryBotIdInitializer itself. I would just read the body as a string and defer JSON parsing to that class:

public async Task Invoke(HttpContext context)
{
    var request = context.Request;

    // Only inspect the request if it's a POST of application/json content
    if (request.Method == "POST"
            &&
        request.GetTypedHeaders().ContentType.MediaType == "application/json")
    {
        // Enable buffering so downstream consumers can also read the body
        request.EnableBuffering();

        var body = request.Body;

        try
        {
            using (var reader = new StreamReader(body, Encoding.UTF8, detectEncodingFromByteOrderMarks: true, bufferSize: 1024, leaveOpen: true))
            {
                context.Items[TelemetryBotIdInitializer.HttpContextItemsKey] = await reader.ReadToEndAsync();
            }
        }
        finally
        {
            // Rewind body stream for downstream consumers
            body.Position = 0;
        }
    }

    await _next(context);
}

Then, as I said, I would worry about pulling the string back out and turning it into a JObject in TelemetryBotIdInitializer.

[drub0y]

See comments on TelemetrySaveBodyASPMiddleware before reading these.

[drub0y]

⚠️ If you were to keep this implementation (which I don't suggest you do)... ⚠️

Remove this, you'll now just pull the value out of the HttpContext::Items collection.

Add the field that will be the key to the collection:

        internal static readonly object HttpContextItemsKey = "__MSBOTFRAMEWORK_TELEMTRYBOTIDINITIALIZER_BODY_JSON_STRING";

[drub0y]

⚠️ If you were to keep this implementation (which I don't suggest you do)... ⚠️

Don't need the memory cache any more, remove from constructor entirely.

[drub0y]

⚠️ If you were to keep this implementation (which I don't suggest you do)... ⚠️

This would now become:

public void Initialize(ITelemetry telemetry)
{
    var httpContext = _httpContextAccessor.HttpContext;

    if (httpContext != null && (telemetry is RequestTelemetry || telemetry is EventTelemetry))
    {
        var body = httpContext.Items[HttpContextItemsKey] as string;

        if (body != null)
        {
            // Parse the JSON stored by the companion ASP.NET middleware so we can extract values from it
            var activity = JObject.Parse(body);

            var userId = activity["from"]?["id"].Value<string>();
            var channelId = activity["channelId"].Value<string>();
            var conversationId = activity["conversation"]?["id"].Value<string>();

            var telemetryContext = telemetry.Context;

            // Set the user id on the Application Insights telemetry item.
            telemetryContext.User.Id = channelId + userId;

            // Set the session id on the Application Insights telemetry item.
            telemetryContext.Session.Id = conversationId;

            // Set the activity id
            telemetryContext.GlobalProperties.Add("activity_id", activity["id"].Value<string>());
        }
    }
}