OTEL implementation, so far matching EventCounters infrastructure. #33633
Conversation
|
@cincuranet @SamMonoRT I know I'm behind on reviewing this, it's high on my list and moving up - early next week for sure. |
|
@cincuranet I just noticed I was marked as a reviewer on this PR. Is there any specific input you'd like me to provide? I don't have a ton of context on efcore. |
Was mostly if you saw patterns from other Aspire OTEL efforts. |
| @@ -152,7 +153,9 @@ public bool MoveNext() | |||
| { | |||
| var sqlQuery = _queryingEnumerable.GenerateQuery(); | |||
|
|
|||
| EntityFrameworkEventSource.Log.QueryExecuting(); | |||
| #pragma warning disable EF1001 // Internal EF Core API usage. | |||
There was a problem hiding this comment.
Is this currently internal because of our plans to evolve the metrics to support database dimensionality (#33634)?
There was a problem hiding this comment.
Yes. And also it does not feel like a public API. It's implementation detail.
| @@ -46,6 +46,7 @@ Microsoft.EntityFrameworkCore.DbSet | |||
| <ItemGroup> | |||
| <PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="$(MicrosoftExtensionsCachingMemoryVersion)" /> | |||
| <PackageReference Include="Microsoft.Extensions.Logging" Version="$(MicrosoftExtensionsLoggingVersion)" /> | |||
| <PackageReference Include="OpenTelemetry" Version="1.8.1" /> | |||
There was a problem hiding this comment.
It isn't great to force this new dependency on all EF users, I'm assuming it's needed for EntityFrameworkCoreInstrumentationMeterProviderBuilderExtensions? Is this the pattern followed by other .NET metrics providers?
(assuming we keep it, extract the version to Versions.props like the others)
There was a problem hiding this comment.
Usually it's part of extension package (like OpenTelemetry.Instrumentation.Http for HtpClient). There's already one OpenTelemetry.Instrumentation.EntityFrameworkCore, but it is implemented over the "old" event source (https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs).
There was a problem hiding this comment.
This dependency should be isolated in a separate package, especially since it's not required for core EF functionality
| @@ -52,4 +52,8 @@ | |||
| <ProjectReference Include="..\EFCore.Specification.Tests\EFCore.Specification.Tests.csproj" /> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="OpenTelemetry.Exporter.InMemory" Version="1.8.1" /> | |||
There was a problem hiding this comment.
Extract version (like for the OpenTelemetry package above)
| namespace Microsoft.EntityFrameworkCore.Infrastructure; | ||
|
|
||
| // The tests interact with global state and should never be run in parallel | ||
| [Collection(nameof(MetricsDataCollection))] |
There was a problem hiding this comment.
If I understood correctly, IMeterFactory is what would allow us to not restrict the parallelism here, maybe open a backlog issue for that...
There was a problem hiding this comment.
Kind of. As long as we keep the EntityFrameworkEventSource and have single logic for both, the IMeterFactory would not work.
But if/when we decide to do #33634, the implementations are going to diverge and we can lift this restriction.
| [InlineData(false)] | ||
| [InlineData(true)] | ||
| public async Task Counts_when_query_is_executed(bool async) | ||
| { | ||
| TotalQueries = 0; | ||
| var initial = EntityFrameworkMetricsData.GetTotalQueriesExecuted(); |
There was a problem hiding this comment.
The tests here seem very similar to those in EntityFrameworkMetricsDataTest - do we need both? This here seems to be more like a unit test (like the previous ones but no reflection), whereas the other ones are a bit more end-to-end, go through the actual OTel machinery - am I understanding things right?
There was a problem hiding this comment.
Correct. The EntityFrameworkMetricsTest goes through OTEL, while EntityFrameworkMetricsDataTest tests only existing implementation. We can probably drop EntityFrameworkMetricsDataTest, if you want.
| @@ -53,7 +47,7 @@ private EntityFrameworkEventSource() | |||
| /// See <see href="https://aka.ms/efcore-docs-event-counters">EF Core event counters</see> for more information and examples. | |||
| /// </remarks> | |||
| public void DbContextInitializing() | |||
| => Interlocked.Increment(ref _activeDbContexts); | |||
| => EntityFrameworkMetricsData.ReportDbContextInitializing(); | |||
There was a problem hiding this comment.
DbContextInitializing() - and all the others here - don't seem to be used any more?
There was a problem hiding this comment.
Backwards compatibility, somebody might be still using EntityFrameworkEventSource.
| private readonly ObservableCounter<long> _saveChangesCounter; | ||
| private readonly ObservableGauge<double> _compiledQueryCacheHitRateGauge; | ||
| private readonly ObservableCounter<long> _executionStrategyOperationFailuresCounter; | ||
| private readonly ObservableCounter<long> _optimisticConcurrencyFailuresCounter; |
There was a problem hiding this comment.
This one may actually make more sense as a percentage gauge, let me know what you think.
There was a problem hiding this comment.
You mean TotalOptimisticConcurrencyFailures/TotalSaveChanges?
Fixes #25880