-
Notifications
You must be signed in to change notification settings - Fork 722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added entityframeworkcore instrumentation #929
added entityframeworkcore instrumentation #929
Conversation
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.
Added some comments. Can you add some tests as well?
...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
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.
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
Show resolved
Hide resolved
...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
Show resolved
Hide resolved
...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
Show resolved
Hide resolved
...trumentation.EntityFrameworkCore/Implementation/EntityFrameworkInstrumentationEventSource.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/PropertyFetcher.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.EntityFrameworkCore/OpenTelemetryBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
Show resolved
Hide resolved
...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
...etry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
…tation/EntityFrameworkDiagnosticListener.cs Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>
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:
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. |
@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. |
|
||
private readonly EntityFrameworkInstrumentationOptions options; | ||
|
||
public EntityFrameworkDiagnosticListener(string sourceName, EntityFrameworkInstrumentationOptions options) |
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.
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.
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.
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 Report
@@ 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
|
using var shutdownSignal = Sdk.CreateTracerProvider(b => | ||
{ | ||
b.AddProcessorPipeline(c => c.AddProcessor(ap => activityProcessor.Object)); | ||
b.AddEntityFrameworkInstrumentation(); |
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.
if we enable SqlClient instrumentation and use SqlClient inside EF, do we get duplicate info - one from EF and one from SqlClient?
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.
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.
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.
Thanks for confirming. This is important to be noted in the readme.md for EF instrumentation and SqlClient instrumentation.
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.
Also unit test this, to make nothing unexpected happens. Expected is 2 spans EF outer span and Sql inner span.
@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. |
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
@cijothomas I spent a little time on this to try and get it over the hump for @Place1. It will now push As far as testing goes...
|
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.
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.
@CodeBlanch , i will start moving this changes to contrib. When i finish, we can close this pr. |
@cijothomas @MikeGoldsmith @CodeBlanch , we can close this, since we already merged in the contrib repo :) |
Fixes #928
Changes
This PR adds
OpenTelemetry.Instrumentation.EntityFrameworkCore
which is modified version ofOpenTelementry.Instrumentation.SqlClient
.This new instrumentation listens to
Microsoft.EntityFrameworkCore.Database.Command.*
diagnostic events to support creating spans for database requests.