Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

Abaranch/request name #17

Merged
merged 4 commits into from
Apr 13, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/ApplicationInsights.AspNet/ActiveConfigurationManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.AspNet.Hosting;
using Microsoft.Framework.ConfigurationModel;
using System;
using Microsoft.Framework.DependencyInjection;

public static class ActiveConfigurationManager
{
Expand Down Expand Up @@ -48,7 +49,7 @@ public static void AddTelemetryInitializers(TelemetryConfiguration aiConfig, ISe
return;
}

var httpAccessor = serviceProvider.GetService(typeof(IHttpContextAccessor)) as IHttpContextAccessor;
var httpAccessor = serviceProvider.GetService<IHttpContextAccessor>();

aiConfig.TelemetryInitializers.Add(new ClientIpHeaderTelemetryInitializer(httpAccessor));
aiConfig.TelemetryInitializers.Add(new UserAgentTelemetryInitializer(httpAccessor));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ public static void AddApplicationInsightsTelemetry(this IServiceCollection servi
rt.Context.InstrumentationKey = svcs.GetService<TelemetryClient>().Context.InstrumentationKey;
return rt;
});

//services.AddScoped<RouteContext>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove it?

}

public static HtmlString ApplicationInsightsJavaScriptSnippet(this IHtmlHelper helper, string instrumentationKey)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ public async Task Invoke(HttpContext httpContext)
telemetry.Duration = sw.Elapsed;
telemetry.ResponseCode = httpContext.Response.StatusCode.ToString();
telemetry.Success = httpContext.Response.StatusCode < 400;

telemetry.HttpMethod = httpContext.Request.Method;

this.telemetryClient.TrackRequest(telemetry);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
namespace Microsoft.ApplicationInsights.AspNet.TelemetryInitializers
{
using System;
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.AspNet.Http;
using Microsoft.AspNet.Hosting;
using Microsoft.AspNet.Http;
using Microsoft.AspNet.Mvc;
using Microsoft.AspNet.Mvc.Routing;
using Microsoft.Framework.DependencyInjection;
using System;
using System.Linq;

public class OperationNameTelemetryInitializer : TelemetryInitializerBase
{
Expand All @@ -15,7 +19,14 @@ protected override void OnInitializeTelemetry(HttpContext platformContext, Reque
{
if (string.IsNullOrEmpty(telemetry.Context.Operation.Name))
{
var name = platformContext.Request.Method + " " + platformContext.Request.Path.Value; // Test potential dangerous request;
string name = this.GetNameFromRouteContext(platformContext.RequestServices);

if (string.IsNullOrEmpty(name))
{
name = platformContext.Request.Path.Value;
}

name = platformContext.Request.Method + " " + name;

var telemetryType = telemetry as RequestTelemetry;
if (telemetryType != null && string.IsNullOrEmpty(telemetryType.Name))
Expand All @@ -26,5 +37,59 @@ protected override void OnInitializeTelemetry(HttpContext platformContext, Reque
telemetry.Context.Operation.Name = name;
}
}

private string GetNameFromRouteContext(IServiceProvider requestServices)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing unit tests for this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I missed... Thank you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I feel that the functional tests, like RequestTelemetryTests, are a trap at this time :)

{
string name = null;

if (requestServices != null)
{
var actionContextAccessor = requestServices.GetService<IScopedInstance<ActionContext>>();

if (actionContextAccessor != null && actionContextAccessor.Value != null &&
actionContextAccessor.Value.RouteData != null && actionContextAccessor.Value.RouteData.Values.Count > 0)
{
var routeValues = actionContextAccessor.Value.RouteData.Values;

object controller;
routeValues.TryGetValue("controller", out controller);
string controllerString = (controller == null) ? string.Empty : controller.ToString();

if (!string.IsNullOrEmpty(controllerString))
{
name = controllerString;

object action;
routeValues.TryGetValue("action", out action);
string actionString = (action == null) ? string.Empty : action.ToString();

if (!string.IsNullOrEmpty(actionString))
{
name += "/" + actionString;
}

if (routeValues.Keys.Count > 2)
{
// Add parameters
var sortedKeys = routeValues.Keys
.Where(key =>
!string.Equals(key, "controller", StringComparison.OrdinalIgnoreCase) &&
!string.Equals(key, "action", StringComparison.OrdinalIgnoreCase) &&
!string.Equals(key, AttributeRouting.RouteGroupKey, StringComparison.OrdinalIgnoreCase))
.OrderBy(key => key, StringComparer.OrdinalIgnoreCase)
.ToArray();

if (sortedKeys.Length > 0)
{
string arguments = string.Join(@"/", sortedKeys);
name += " [" + arguments + "]";
}
}
}
}
}

return name;
}
}
}
24 changes: 16 additions & 8 deletions test/FunctionalTestUtils/InProcessServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
using Microsoft.Framework.DependencyInjection.Fallback;
using System;

public class InProcessServer
public class InProcessServer : IDisposable
{
private IDisposable hostingEngine;
private string url = "http://localhost:" + (new Random(239).Next(5000, 8000)).ToString();
private string assemblyName = "ConsoleTest";

public InProcessServer(string currentAssemblyName)

public InProcessServer(string assemblyName)
{
this.assemblyName = currentAssemblyName;
this.Start(assemblyName);
}

public string BaseHost
Expand All @@ -24,7 +24,7 @@ public string BaseHost
}
}

public IDisposable Start()
private void Start(string assemblyName)
{
var customConfig = new NameValueConfigurationSource();
customConfig.Set("server.urls", this.BaseHost);
Expand All @@ -39,7 +39,7 @@ public IDisposable Start()
Services = services,
Configuration = config,
ServerName = "Microsoft.AspNet.Server.WebListener",
ApplicationName = this.assemblyName,
ApplicationName = assemblyName,
EnvironmentName = "Production"
};

Expand All @@ -49,7 +49,15 @@ public IDisposable Start()
throw new Exception("TODO: IHostingEngine service not available exception");
}

return hostingEngine.Start(context);
this.hostingEngine = hostingEngine.Start(context);
}

public void Dispose()
{
if (this.hostingEngine != null)
{
this.hostingEngine.Dispose();
}
}
}
}
35 changes: 35 additions & 0 deletions test/FunctionalTestUtils/Tests/RequestTelemetryTestsBase.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.DataContracts;
using System;
using System.Net;
using Xunit;

namespace FunctionalTestUtils.Tests
{
public abstract class RequestTelemetryTestsBase : TelemetryTestsBase
{
public RequestTelemetryTestsBase(string assemblyName) : base(assemblyName)
{ }

public void ValidateBasicRequest(string requestPath, RequestTelemetry expected)
{
DateTimeOffset testStart = DateTimeOffset.Now;
var task = this.HttpClient.GetAsync(this.Server.BaseHost + requestPath);
task.Wait(TestTimeoutMs);
var result = task.Result;

Assert.Equal(1, this.Buffer.Count);
ITelemetry telemetry = this.Buffer[0];
Assert.IsAssignableFrom(typeof(RequestTelemetry), telemetry);
var actual = (RequestTelemetry)telemetry;

Assert.Equal(expected.ResponseCode, actual.ResponseCode);
Assert.Equal(expected.Name, actual.Name);
Assert.Equal(expected.Success, actual.Success);
Assert.Equal(expected.Url, actual.Url);
Assert.InRange<DateTimeOffset>(actual.Timestamp, testStart, DateTimeOffset.Now);
Assert.True(actual.Duration < (DateTimeOffset.Now - testStart), "duration");
Assert.Equal(expected.HttpMethod, actual.HttpMethod);
}
}
}
60 changes: 60 additions & 0 deletions test/FunctionalTestUtils/Tests/TelemetryTestsBase.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
namespace FunctionalTestUtils.Tests
{
using Microsoft.ApplicationInsights.Channel;
using System;
using System.Collections.Generic;
using System.Net.Http;

public abstract class TelemetryTestsBase : IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test class hierarchy three layers deep, with only one class in each layer feels over-engineered. Are we over-engineering here and/or breaking the YAGNI principle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. And no it is not. I was adding one more test application that would call same tests that in the current application. The reason it is not added right away is that tests from WebApi app are not executed.

{
protected const int TestTimeoutMs = 5000;

private InProcessServer server;
private IList<ITelemetry> buffer = new List<ITelemetry>();
private HttpClient client = new HttpClient();

public TelemetryTestsBase(string assemblyName)
{
this.server = new InProcessServer(assemblyName);
BackTelemetryChannelExtensions.InitializeFunctionalTestTelemetryChannel(buffer);
BackTelemetryChannelExtensions.AddFunctionalTestTelemetryChannel(null);
}

public IList<ITelemetry> Buffer
{
get
{
return this.buffer;
}
}

public InProcessServer Server
{
get
{
return this.server;
}
}

public HttpClient HttpClient
{
get
{
return this.client;
}
}

public void Dispose()
{
if (this.server != null)
{
this.server.Dispose();
}

if (this.client != null)
{
this.client.Dispose();
}
}
}
}
4 changes: 2 additions & 2 deletions test/SampleWebAppIntegration/Controllers/HomeController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ public IActionResult Index()
return View();
}

public IActionResult About()
public IActionResult About(int index)
{
ViewBag.Message = "Your application description page.";
ViewBag.Message = "Your application description page # " + index;

return View();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
namespace SampleWebAppIntegration.FunctionalTest
{
using FunctionalTestUtils.Tests;
using Microsoft.ApplicationInsights.DataContracts;
using System.Linq;
using Xunit;

public class RequestTelemetryTests : RequestTelemetryTestsBase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that these tests set us on a path of using end-to-end tests over unit tests. This already created a problem when I was merging a separate pull request. Product code was fine, but these tests weren't passing because the test infrastructure had to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No they are not. We have both unit test and basic E2E tests. And we need both. These test validate basic request properties for a specific application type. In this case MVC 6. The plan was to add WebAPI and reuse same tests there.

{
public RequestTelemetryTests() : base("SampleWebAppIntegration")
{ }

[Fact]
public void TestBasicRequestPropertiesAfterRequestingHomeController()
{
var expectedRequestTelemetry = new RequestTelemetry();
expectedRequestTelemetry.HttpMethod = "GET";
expectedRequestTelemetry.Name = "GET Home/Index";
expectedRequestTelemetry.ResponseCode = "200";
expectedRequestTelemetry.Success = true;
// expectedRequestTelemetry.Url ???

this.ValidateBasicRequest("/", expectedRequestTelemetry);
}

[Fact]
public void TestBasicRequestPropertiesAfterRequestingActionWithParameter()
{
var expectedRequestTelemetry = new RequestTelemetry();
expectedRequestTelemetry.HttpMethod = "GET";
expectedRequestTelemetry.Name = "GET Home/About [id]";
expectedRequestTelemetry.ResponseCode = "200";
expectedRequestTelemetry.Success = true;
// expectedRequestTelemetry.Url ???

this.ValidateBasicRequest("/Home/About/5", expectedRequestTelemetry);
}

[Fact]
public void TestBasicRequestPropertiesAfterRequestingNotExistingController()
{
var expectedRequestTelemetry = new RequestTelemetry();
expectedRequestTelemetry.HttpMethod = "GET";
expectedRequestTelemetry.Name = "GET /not/existing/controller";
expectedRequestTelemetry.ResponseCode = "404";
expectedRequestTelemetry.Success = false;

this.ValidateBasicRequest("/not/existing/controller", expectedRequestTelemetry);
}

[Fact]
public void TestMixedTelemetryItemsReceived()
{
var task = this.HttpClient.GetAsync(this.Server.BaseHost + "/home/contact");
task.Wait(TestTimeoutMs);

var request = this.Buffer.OfType<RequestTelemetry>().Single();
var eventTelemetry = this.Buffer.OfType<EventTelemetry>().Single();
var metricTelemetry = this.Buffer.OfType<MetricTelemetry>().Single();
var traceTelemetry = this.Buffer.OfType<TraceTelemetry>().Single();

Assert.Equal(4, this.Buffer.Count);
Assert.NotNull(request);
Assert.NotNull(eventTelemetry);
Assert.NotNull(metricTelemetry);
Assert.NotNull(traceTelemetry);
}
}
}
Loading