Add EntityFramework diagnostics source #297

pakrym · 2016-12-12T23:53:45Z

Also reference individual packages instead AspNetCore.Hosting as per comment in my previous PR.

msftclas · 2016-12-12T23:53:48Z

Hi @pakrym, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

SergeyKanzhelev · 2016-12-13T05:36:16Z

Last we discussed this should go to Dependency collector nugget. What changed?

pakrym · 2016-12-13T05:59:02Z

@SergeyKanzhelev we were discussing SqlClient tracing, this diagnostic source can be used with any database client that has entity framework adapter.

dnduffy · 2016-12-13T18:22:18Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+    using Microsoft.Extensions.DiagnosticAdapter;
+
+    /// <summary>
+    /// <see cref="IApplicationInsightDiagnosticListener"/> implementation that listens for evens specific to EntiryFrameworkCore


evens = events
nitpick: period on sentence.

dnduffy · 2016-12-13T18:23:50Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+        private readonly TelemetryClient client;
+        private readonly ContextData<long> beginDependencyTimestamp = new ContextData<long>();
+
+        public string ListenerName { get; } = "Microsoft.EntityFrameworkCore";


nitpick: doc comment "/// Gets the listener name."
And doc comments elsewhere.

dnduffy · 2016-12-13T18:26:55Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+            telemetry.Name = command.Connection.Database;
+            telemetry.Data = command.CommandText;
+            telemetry.Duration = new TimeSpan(end - start);
+            telemetry.Timestamp = DateTimeOffset.Now - telemetry.Duration;


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.

You want me to update all occurrences?

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.

dnduffy · 2016-12-13T18:28:12Z

...oft.ApplicationInsights.AspNetCore/Extensibility/Implementation/Tracing/AspNetEventSource.cs

@@ -3,6 +3,9 @@
 //     Copyright (c) Microsoft Corporation. All rights reserved.
 // </copyright>
 //-----------------------------------------------------------------------
+
+using System.Reflection;


Can this using go inside the namespace for consistency? I don't think it is needed for anything outside the namespace.

dnduffy · 2016-12-13T18:28:54Z

src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs

@@ -1,4 +1,6 @@
-namespace Microsoft.Extensions.DependencyInjection
+using Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners.Implementation;


Please place inside the namespace sorted with the other usings.

dnduffy · 2016-12-13T18:30:25Z

test/MVCFramework45.FunctionalTests/FunctionalTest/EntityFrameworkTelemetryTests.cs

+                    task.Wait(TestTimeoutMs);
+                }
+            }
+            var telemetries = server.BackChannel.Buffer.OfType<DependencyTelemetry>()


What is the type here? I'm guessing it might be "DependencyTelemetry[]" but I'm not sure, please replace var with the known type.

dnduffy · 2016-12-13T18:33:21Z

test/Microsoft.ApplicationInsights.AspNetCore.Tests/EntityFrameworkDiagnosticListenerTest.cs

+            Assert.Equal(new TimeSpan(100000), sentTelemetry.Duration);
+        }
+
+        public class MockDbCommand : System.Data.IDbCommand


nitpick: I know it's test code but I would love doc comments anyway please.

do you want doc comment for members too?

dnduffy

I would like some minor tweaks, mostly doc comments, and then I'll merge it. Thank-you. :)

SergeyKanzhelev · 2016-12-13T20:43:21Z

@pakrym - In Application Insights we split telemetry collection logic by types. Typically dependencies will be collected by DependencyCollector package.

I do not understand why we need to collect EntityFramework collection right in AspNet package?

SergeyKanzhelev · 2016-12-13T20:44:18Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+            var start = beginDependencyTimestamp.Value;
+            var end = timestamp;
+
+            var telemetry = new DependencyTelemetry();


How does it work with SQL dependencies collected by DependencyCollector module? Will we have double counting?

Only for SqlClient on full framework.

That would be a problem.

Options? Only enable this feature for .NET Core? Double counting is not good

This is useful for non SqlClient connection on full framework too. We can try and filter out SqlCommands by type.

SergeyKanzhelev · 2016-12-13T20:45:06Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+            var end = timestamp;
+
+            var telemetry = new DependencyTelemetry();
+            telemetry.Name = command.Connection.Database;


Name should be a name of stored procedure or friendly name of SQL command. Not the database name

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

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

So it's, Server | Database | StoredProcedure for stored procedures and Server | Database for everything else. But DataSource unfortunately is SqlClient specific.

SergeyKanzhelev · 2016-12-13T20:45:21Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+            telemetry.Data = command.CommandText;
+            telemetry.Duration = new TimeSpan(end - start);
+            telemetry.Timestamp = DateTimeOffset.Now - telemetry.Duration;
+            telemetry.Target = command.Connection.Database;


Do we have Server name? Please include it into target

No, because this is provider agnostic there is no common way to get a server name.

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

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.

SergeyKanzhelev · 2016-12-14T18:44:09Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+            var start = beginDependencyTimestamp.Value;
+            var end = timestamp;
+
+            var telemetry = new DependencyTelemetry();


Options? Only enable this feature for .NET Core? Double counting is not good

SergeyKanzhelev · 2016-12-14T18:50:32Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+            var end = timestamp;
+
+            var telemetry = new DependencyTelemetry();
+            telemetry.Name = command.Connection.Database;


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

SergeyKanzhelev · 2016-12-14T18:51:32Z

...nInsights.AspNetCore/DiagnosticListeners/Implementation/EntityFrameworkDiagnosticListener.cs

+            telemetry.Data = command.CommandText;
+            telemetry.Duration = new TimeSpan(end - start);
+            telemetry.Timestamp = DateTimeOffset.Now - telemetry.Duration;
+            telemetry.Target = command.Connection.Database;


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

pakrym added 3 commits December 12, 2016 15:05

Add EntityFramework diagnostics listener

9f132bf

Add functional test

44f1a8d

Fix namespaces

4a0dd92

msftclas added the cla-required label Dec 12, 2016

namespaces in tests

5d805b8

dnduffy reviewed Dec 13, 2016

View reviewed changes

dnduffy approved these changes Dec 13, 2016

View reviewed changes

AlexBulankou added this to the 2.0.0-Beta2 milestone Dec 13, 2016

PR comments

251fa0d

SergeyKanzhelev reviewed Dec 13, 2016

View reviewed changes

SergeyKanzhelev suggested changes Dec 13, 2016

View reviewed changes

SergeyKanzhelev suggested changes Dec 14, 2016

View reviewed changes

pakrym closed this Feb 17, 2017

AlexBulankou mentioned this pull request Apr 28, 2017

Add support for EntityFramework diagnostic source listener microsoft/ApplicationInsights-dotnet-server#522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EntityFramework diagnostics source #297

Add EntityFramework diagnostics source #297

pakrym commented Dec 12, 2016

msftclas commented Dec 12, 2016

SergeyKanzhelev commented Dec 13, 2016

pakrym commented Dec 13, 2016

dnduffy Dec 13, 2016 •

edited

dnduffy Dec 13, 2016 •

edited

dnduffy Dec 13, 2016

pakrym Dec 13, 2016

dnduffy Dec 13, 2016

dnduffy Dec 13, 2016

dnduffy Dec 13, 2016

dnduffy Dec 13, 2016

dnduffy Dec 13, 2016

pakrym Dec 13, 2016

dnduffy left a comment

SergeyKanzhelev commented Dec 13, 2016

SergeyKanzhelev Dec 13, 2016

pakrym Dec 13, 2016

dnduffy Dec 13, 2016

SergeyKanzhelev Dec 14, 2016

pakrym Dec 14, 2016

SergeyKanzhelev Dec 13, 2016

pakrym Dec 13, 2016

SergeyKanzhelev Dec 14, 2016

pakrym Dec 14, 2016

SergeyKanzhelev Dec 13, 2016

pakrym Dec 13, 2016

SergeyKanzhelev Dec 14, 2016

pakrym Dec 14, 2016

SergeyKanzhelev Dec 14, 2016

SergeyKanzhelev Dec 14, 2016

SergeyKanzhelev Dec 14, 2016

		@@ -1,4 +1,6 @@
		namespace Microsoft.Extensions.DependencyInjection
		using Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners.Implementation;

Add EntityFramework diagnostics source #297

Add EntityFramework diagnostics source #297

Conversation

pakrym commented Dec 12, 2016

msftclas commented Dec 12, 2016

SergeyKanzhelev commented Dec 13, 2016

pakrym commented Dec 13, 2016

dnduffy Dec 13, 2016 • edited

Choose a reason for hiding this comment

dnduffy Dec 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnduffy left a comment

Choose a reason for hiding this comment

SergeyKanzhelev commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnduffy Dec 13, 2016 •

edited

dnduffy Dec 13, 2016 •

edited