-
Notifications
You must be signed in to change notification settings - Fork 123
Add EntityFramework diagnostics source #297
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
namespace Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners.Implementation | ||
{ | ||
using System; | ||
using Microsoft.ApplicationInsights.DataContracts; | ||
using Microsoft.Extensions.DiagnosticAdapter; | ||
|
||
/// <summary> | ||
/// <see cref="IApplicationInsightDiagnosticListener"/> implementation that listens for events specific to EntiryFrameworkCore. | ||
/// </summary> | ||
public class EntityFrameworkDiagnosticListener : IApplicationInsightDiagnosticListener | ||
{ | ||
private readonly TelemetryClient client; | ||
private readonly ContextData<long> beginDependencyTimestamp = new ContextData<long>(); | ||
|
||
/// <inheritdoc /> | ||
public string ListenerName { get; } = "Microsoft.EntityFrameworkCore"; | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="T:HostingDiagnosticListener"/> class. | ||
/// </summary> | ||
/// <param name="client"><see cref="TelemetryClient"/> to post traces to.</param> | ||
public EntityFrameworkDiagnosticListener(TelemetryClient client) | ||
{ | ||
this.client = client; | ||
} | ||
|
||
/// <summary> | ||
/// Diagnostic event handler method for 'Microsoft.EntityFrameworkCore.BeforeExecuteCommand' event | ||
/// </summary> | ||
[DiagnosticName("Microsoft.EntityFrameworkCore.BeforeExecuteCommand")] | ||
public void OnBeginCommand(long timestamp) | ||
{ | ||
beginDependencyTimestamp.Value = timestamp; | ||
} | ||
|
||
/// <summary> | ||
/// Diagnostic event handler method for 'Microsoft.EntityFrameworkCore.AfterExecuteCommand' event | ||
/// </summary> | ||
[DiagnosticName("Microsoft.EntityFrameworkCore.AfterExecuteCommand")] | ||
public void OnEndCommand(IDbCommand command, string executeMethod, Guid instanceId, long timestamp, bool isAsync) | ||
{ | ||
var start = beginDependencyTimestamp.Value; | ||
var end = timestamp; | ||
|
||
var telemetry = new DependencyTelemetry(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does it work with SQL dependencies collected by DependencyCollector module? Will we have double counting? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only for SqlClient on full framework. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be a problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Options? Only enable this feature for .NET Core? Double counting is not good There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is useful for non SqlClient connection on full framework too. We can try and filter out SqlCommands by type. |
||
telemetry.Name = command.Connection.Database; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name should be a name of stored procedure or friendly name of SQL command. Not the database name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I see it just puts entire command text in there: https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/Src/DependencyCollector/Shared/Implementation/FrameworkSqlProcessing.cs#L128 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Framework only sets command text as a name for stored procedures. We have quite strange logic for full framework: https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/Src/DependencyCollector/Shared/Implementation/ProfilerSqlProcessing.cs#L144 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it's, |
||
telemetry.Data = command.CommandText; | ||
telemetry.Duration = new TimeSpan(end - start); | ||
telemetry.Timestamp = DateTimeOffset.Now - telemetry.Duration; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We had a discussion the other day that we should probably go through our code and replace all instances of DateTimeOffset.Now with DateTimeOffset.UtcNow because there happens to be a perf difference between them so maybe this one should be updated now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You want me to update all occurrences? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just thinking of this one that is new, or you could leave it and I'll get it with all the others, probably safe to leave it. Sorry, I just commented on it when I saw it but probably I should do it with the others all at once so don't worry about it. |
||
telemetry.Target = command.Connection.Database; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have Server name? Please include it into target There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because this is provider agnostic there is no common way to get a server name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make our best attempt? Like parse connection string or something. We want to light up scenario in UI when we open database information from the application map's SQL node There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can send entire connection string but it might contain secrets. We can parse connection string syntax but we don't know what to search for. |
||
telemetry.Type = "SQL"; | ||
client.TrackDependency(telemetry); | ||
} | ||
|
||
/// <summary> | ||
/// Proxy interface for <c>IDbCommand</c> from System.Data | ||
/// </summary> | ||
public interface IDbCommand | ||
{ | ||
string CommandText { get; } | ||
|
||
IDbConnection Connection { get; } | ||
} | ||
|
||
/// <summary> | ||
/// Proxy interface for <c>IDbConnection</c> from System.Data | ||
/// </summary> | ||
public interface IDbConnection | ||
{ | ||
string Database { get; } | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
namespace MVCFramework45.FunctionalTests.FunctionalTest | ||
{ | ||
using System.Linq; | ||
using System.Net.Http; | ||
using FunctionalTestUtils; | ||
using Microsoft.ApplicationInsights.DataContracts; | ||
using Xunit; | ||
|
||
public class EntityFrameworkTelemetryTests : TelemetryTestsBase | ||
{ | ||
private const string assemblyName = "MVCFramework45.FunctionalTests"; | ||
|
||
[Fact] | ||
public void TestEntityFrameworkTelemetryItemsReceived() | ||
{ | ||
InProcessServer server; | ||
using (server = new InProcessServer(assemblyName, InProcessServer.UseApplicationInsights)) | ||
{ | ||
using (var httpClient = new HttpClient()) | ||
{ | ||
var task = httpClient.GetAsync(server.BaseHost + "/Home/Contact"); | ||
task.Wait(TestTimeoutMs); | ||
} | ||
} | ||
DependencyTelemetry[] telemetries = server.BackChannel.Buffer.OfType<DependencyTelemetry>() | ||
.Where(t => t.Type == "SQL" && t.Target == "aspnet-MVCFramework45.FunctionalTests-60cfc765-2dc9-454c-bb34-dc379ed92cd0") | ||
.ToArray(); | ||
|
||
Assert.True(telemetries.Length >= 2); | ||
Assert.All(telemetries, telemetry => | ||
{ | ||
Assert.StartsWith("SELECT ", telemetry.Data); | ||
Assert.Equal(telemetry.Target, telemetry.Name); | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,164 @@ | ||
namespace Microsoft.ApplicationInsights.AspNetCore.Tests | ||
{ | ||
using System; | ||
using System.Data; | ||
using System.Diagnostics; | ||
using Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners.Implementation; | ||
using Microsoft.ApplicationInsights.AspNetCore.Tests.Helpers; | ||
using Microsoft.ApplicationInsights.DataContracts; | ||
using Xunit; | ||
|
||
public class EntityFrameworkDiagnosticListenerTest | ||
{ | ||
private const string TestListenerName = "TestListener"; | ||
|
||
private DependencyTelemetry sentTelemetry; | ||
|
||
private DiagnosticListener telemetryListener; | ||
|
||
public EntityFrameworkDiagnosticListenerTest() | ||
{ | ||
var listener = new EntityFrameworkDiagnosticListener(CommonMocks.MockTelemetryClient(telemetry => this.sentTelemetry = (DependencyTelemetry)telemetry)); | ||
telemetryListener = new DiagnosticListener(TestListenerName); | ||
telemetryListener.SubscribeWithAdapter(listener); | ||
} | ||
|
||
[Fact] | ||
public void FillsTelemetryProperties() | ||
{ | ||
var datetime = DateTimeOffset.Now; | ||
var timestamp = Stopwatch.GetTimestamp(); | ||
telemetryListener.Write("Microsoft.EntityFrameworkCore.BeforeExecuteCommand", | ||
new { timestamp }); | ||
telemetryListener.Write("Microsoft.EntityFrameworkCore.AfterExecuteCommand", | ||
new | ||
{ | ||
timestamp = timestamp + 100000, | ||
command = new MockDbCommand() | ||
{ | ||
Connection = new MockDbConnection() | ||
{ | ||
Database = "Database name" | ||
}, | ||
CommandText = "Command text" | ||
} | ||
}); | ||
|
||
Assert.Equal("Database name", sentTelemetry.Name); | ||
Assert.Equal("Command text", sentTelemetry.Data); | ||
Assert.Equal("SQL", sentTelemetry.Type); | ||
Assert.True(datetime < sentTelemetry.Timestamp); | ||
Assert.Equal(new TimeSpan(100000), sentTelemetry.Duration); | ||
} | ||
|
||
|
||
/// <summary> | ||
/// Mock class for <see cref="IDbCommand"/> interface. | ||
/// </summary> | ||
public class MockDbCommand : System.Data.IDbCommand | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: I know it's test code but I would love doc comments anyway please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you want doc comment for members too? |
||
{ | ||
public string CommandText { get; set; } | ||
|
||
public int CommandTimeout { get; set; } | ||
|
||
public CommandType CommandType { get; set; } | ||
|
||
public System.Data.IDbConnection Connection { get; set; } | ||
|
||
public IDataParameterCollection Parameters { get; set; } | ||
|
||
public IDbTransaction Transaction { get; set; } | ||
|
||
public UpdateRowSource UpdatedRowSource { get; set; } | ||
|
||
public void Cancel() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public IDbDataParameter CreateParameter() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public int ExecuteNonQuery() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public IDataReader ExecuteReader() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public IDataReader ExecuteReader(CommandBehavior behavior) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public object ExecuteScalar() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public void Prepare() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Mock class for <see cref="IDbConnection"/> interface. | ||
/// </summary> | ||
public class MockDbConnection : System.Data.IDbConnection | ||
{ | ||
public void Dispose() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public IDbTransaction BeginTransaction() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public IDbTransaction BeginTransaction(IsolationLevel il) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public void Close() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public IDbCommand CreateCommand() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public void Open() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public string ConnectionString { get; set; } | ||
|
||
public int ConnectionTimeout { get; set; } | ||
|
||
public string Database { get; set; } | ||
|
||
public ConnectionState State { get; set; } | ||
|
||
public void ChangeDatabase(string databaseName) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: doc comment "/// Gets the listener name."
And doc comments elsewhere.