added entityframeworkcore instrumentation by Place1 · Pull Request #929 · open-telemetry/opentelemetry-dotnet

Place1 · 2020-07-27T01:01:24Z

Fixes #928

Changes

This PR adds OpenTelemetry.Instrumentation.EntityFrameworkCore which is modified version of OpenTelementry.Instrumentation.SqlClient.

This new instrumentation listens to Microsoft.EntityFrameworkCore.Database.Command.* diagnostic events to support creating spans for database requests.

linux-foundation-easycla · 2020-07-27T01:01:28Z

The committers are authorized under a signed CLA.

✅ James Batt (9aa6dfd, 9283598, 4e925fc, dd04131, d87706f, cc67375, 0391d59)

eddynaka

Added some comments. Can you add some tests as well?

...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs

MikeGoldsmith

Looks like a great start - thanks! I've left some minor changes to clean up the commit.

Also, you'll need to sign the CLA before we can merge the PR 👍

...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs

...trumentation.EntityFrameworkCore/Implementation/EntityFrameworkInstrumentationEventSource.cs

src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/PropertyFetcher.cs

src/OpenTelemetry.Instrumentation.EntityFrameworkCore/OpenTelemetryBuilderExtensions.cs

...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs

…tation/EntityFrameworkDiagnosticListener.cs Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>

Place1 · 2020-07-27T09:25:52Z

Added some comments. Can you add some tests as well?

Adding tests seems difficult and is more of a time commitment than I had in mind here. I don't actually use Open Telemetry yet (but it's a great project that i hope to use 😄).

I guess tests would need to run against a few of EF Core's supported backends:

sqlite3
postgres
mysql
mssql

I don't think I have the time to write all of this testing code. Apologies.

I'm happy for someone to take ownership of this PR.

CodeBlanch · 2020-07-27T16:40:19Z

@Place1 Do you happen to have a link handy for the EF Core code sending the diagnostic source events? I just wanted to check out the other side of the equation to see how they are doing it, look if there's anything more interesting we can push into the data, etc.

CodeBlanch · 2020-07-27T16:46:05Z

...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs

+
+        private readonly EntityFrameworkInstrumentationOptions options;
+
+        public EntityFrameworkDiagnosticListener(string sourceName, EntityFrameworkInstrumentationOptions options)


I see one potential issue. The SqlClient .NET code does not start Activities, it just fires events. This is why the SqlClientDiagnosticListener has SupportsNullActivity => true and is written to create Activity, not just populate them like some of the other instrumentation. This class is missing SupportsNullActivity => true but is still written in that style to create Activity always. Probably a mismatch somewhere. I need to go look at the EF code to figure out the correct behavior, will do later today.

Update: I went digging through the source. EF does not spin up Activity:
https://github.com/dotnet/efcore/blob/7d6cb86ff1facb1480a51519f8052043977831bc/src/EFCore.Relational/Diagnostics/RelationalLoggerExtensions.cs#L235-L238

So the code needed SupportsNullActivity => true. Since I was in there, I built a unit test on top of Sqlite in-memory DB. That led to some other bug fixes. Sorry @Place1 for jumping in there, I hope you don't mind.

I just pushed what I have so far. Could probably use a bit more unit testing and maybe some enhancements to the data being collected, but working well-enough I think we can merge PR and do that separate? Or move it to contrib?

codecov · 2020-07-28T06:23:22Z

Codecov Report

Merging #929 into master will decrease coverage by 0.59%.
The diff coverage is 46.54%.

@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
- Coverage   68.74%   68.14%   -0.60%     
==========================================
  Files         213      219       +6     
  Lines        5912     6068     +156     
  Branches      967      997      +30     
==========================================
+ Hits         4064     4135      +71     
- Misses       1580     1643      +63     
- Partials      268      290      +22

Impacted Files	Coverage Δ
...eworkCore/EntityFrameworkInstrumentationOptions.cs	`0.00% <0.00%> (ø)`
...ation/EntityFrameworkInstrumentationEventSource.cs	`0.00% <0.00%> (ø)`
...mplementation/EntityFrameworkDiagnosticListener.cs	`44.08% <44.08%> (ø)`
...ityFrameworkCore/OpenTelemetryBuilderExtensions.cs	`57.14% <57.14%> (ø)`
...ityFrameworkCore/Implementation/PropertyFetcher.cs	`66.66% <66.66%> (ø)`
...ityFrameworkCore/EntityFrameworkInstrumentation.cs	`88.88% <88.88%> (ø)`
...ient/Implementation/SqlClientDiagnosticListener.cs	`61.53% <100.00%> (ø)`
... and 3 more

cijothomas · 2020-07-28T14:04:57Z

...elemetry.Instrumentation.EntityFrameworkCore.Tests/EntityFrameworkDiagnosticListenerTests.cs

+            using var shutdownSignal = Sdk.CreateTracerProvider(b =>
+            {
+                b.AddProcessorPipeline(c => c.AddProcessor(ap => activityProcessor.Object));
+                b.AddEntityFrameworkInstrumentation();


if we enable SqlClient instrumentation and use SqlClient inside EF, do we get duplicate info - one from EF and one from SqlClient?

Yes in that case you get an outer span from EF instrumentation and an inner span from Sql instrumentation. I just tested to be sure.

Thanks for confirming. This is important to be noted in the readme.md for EF instrumentation and SqlClient instrumentation.

Also unit test this, to make nothing unexpected happens. Expected is 2 spans EF outer span and Sql inner span.

cijothomas · 2020-07-29T04:49:29Z

@Place1 Let us know if you have time to make requested changes/tests. If not, we'll try to add them and merge. Please let us know.

Place1 · 2020-07-29T05:07:02Z

I apologise but I don’t have any time for the next 2-3 weeks. Im happy for others to take over here.

…

On Wed, 29 Jul 2020 at 2:49 pm, Cijo Thomas ***@***.***> wrote: @Place1 <https://github.com/Place1> Let us know if you have time to make requested changes/tests. If not, we'll try to add them and merge. Please let us know. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#929 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACZQDFS4PRIYD3FOOTFPLADR56S6PANCNFSM4PIIDFCA> .

…b.com/Place1/opentelemetry-dotnet into entity-framework-core-instrumentation

CodeBlanch · 2020-07-30T06:02:35Z

@cijothomas I spent a little time on this to try and get it over the hump for @Place1. It will now push db.system using the ProviderName exposed by EF. There are a lot of providers. I picked what I felt were the top ones. I thought about adding an option so users can take over that decision if their provider isn't in our list? Let me know your thoughts on that.

As far as testing goes...

After Runtime context API #948 goes in we can use that to stop the double-collection case? I don't want to spend a bunch of time adding tests for it, would rather spend that time fixing the issue.
I added a test for exception case.
I tried to add a test for stored procedure case, but not supported by sqlite.
I don't have it in me to add tests for every provider EF supports. Open to ideas on this. Maybe we should move this to contrib and designate as quasi-experimental?

MikeGoldsmith

I think this looks good now. We can expand the known provider list if requested, I don't think we need to expose a hook for users to override it.

eddynaka · 2020-07-30T14:14:17Z

@cijothomas I spent a little time on this to try and get it over the hump for @Place1. It will now push db.system using the ProviderName exposed by EF. There are a lot of providers. I picked what I felt were the top ones. I thought about adding an option so users can take over that decision if their provider isn't in our list? Let me know your thoughts on that.

As far as testing goes...

After Runtime context API #948 goes in we can use that to stop the double-collection case? I don't want to spend a bunch of time adding tests for it, would rather spend that time fixing the issue.

I added a test for exception case.

I tried to add a test for stored procedure case, but not supported by sqlite.

I don't have it in me to add tests for every provider EF supports. Open to ideas on this. Maybe we should move this to contrib and designate as quasi-experimental?

@CodeBlanch , i will start moving this changes to contrib. When i finish, we can close this pr.

eddynaka · 2020-07-31T10:21:31Z

@cijothomas @MikeGoldsmith @CodeBlanch , we can close this, since we already merged in the contrib repo :)

added entityframeworkcore instrumentation

9aa6dfd

Place1 requested a review from a team July 27, 2020 01:01

Place1 mentioned this pull request Jul 27, 2020

Npgsql Postgresql client instrumentation #928

Closed

eddynaka reviewed Jul 27, 2020

View reviewed changes

MikeGoldsmith suggested changes Jul 27, 2020

View reviewed changes

Place1 added 6 commits July 27, 2020 18:06

fixed activity source name

9283598

moved some code

4e925fc

null checks

dd04131

updated event source name

d87706f

wrapped code in a try catch

cc67375

renamed variable

0391d59

eddynaka reviewed Jul 27, 2020

View reviewed changes

...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs Show resolved Hide resolved

eddynaka reviewed Jul 27, 2020

View reviewed changes

...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs Outdated Show resolved Hide resolved

eddynaka reviewed Jul 27, 2020

View reviewed changes

...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs Outdated Show resolved Hide resolved

Place1 and others added 3 commits July 27, 2020 19:11

Update src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implemen…

b342695

…tation/EntityFrameworkDiagnosticListener.cs Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>

removed null checks in favour of initializing empty options

c5cfa29

fixed blank line

2415714

CodeBlanch reviewed Jul 27, 2020

View reviewed changes

Bug fixes and a unit test.

10d7a91

cijothomas reviewed Jul 28, 2020

View reviewed changes

Merge branch 'master' into entity-framework-core-instrumentation

6d1495f

cijothomas mentioned this pull request Jul 29, 2020

EF Core support #79

Closed

cijothomas and others added 2 commits July 29, 2020 19:02

Merge branch 'master' into entity-framework-core-instrumentation

be2ee52

Constants.

fae8d2a

CodeBlanch added 3 commits July 29, 2020 21:36

Merge branch 'entity-framework-core-instrumentation' of https://githu…

f11286c

…b.com/Place1/opentelemetry-dotnet into entity-framework-core-instrumentation

Setting db.system tag from EF ProviderName.

32a3721

Added test case for errors.

dd6177b

MikeGoldsmith approved these changes Jul 30, 2020

View reviewed changes

Merge branch 'master' into entity-framework-core-instrumentation

4b1fce3

eddynaka mentioned this pull request Jul 30, 2020

Adding EFCore Instrumentation open-telemetry/opentelemetry-dotnet-contrib#18

Merged

CodeBlanch closed this Jul 31, 2020

jzabroski mentioned this pull request Oct 27, 2022

Allow query tags to be forced inside generated SQL to prevent SQL Server from stripping them from "query text entries" dotnet/efcore#20105

Closed


		private readonly EntityFrameworkInstrumentationOptions options;

		public EntityFrameworkDiagnosticListener(string sourceName, EntityFrameworkInstrumentationOptions options)

Conversation

Place1 commented Jul 27, 2020

Changes

Uh oh!

linux-foundation-easycla bot commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddynaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MikeGoldsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Place1 commented Jul 27, 2020

Uh oh!

CodeBlanch commented Jul 27, 2020

Uh oh!

CodeBlanch Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

CodeBlanch Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cijothomas Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

CodeBlanch Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

cijothomas Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

cijothomas Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

cijothomas commented Jul 29, 2020

Uh oh!

Place1 commented Jul 29, 2020 via email

Uh oh!

CodeBlanch commented Jul 30, 2020

Uh oh!

MikeGoldsmith left a comment

Choose a reason for hiding this comment

Uh oh!

eddynaka commented Jul 30, 2020

Uh oh!

eddynaka commented Jul 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

linux-foundation-easycla bot commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 28, 2020 •

edited

Loading