Skip to content
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.

Sign up for GitHub

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

Fix database issues #630

Merged
merged 3 commits into from
Nov 1, 2023
Merged

Fix database issues #630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da24791
Allow for configuring DbContextOptions in EF Components
eerhardt Nov 1, 2023
6dcbe40
Enable Npgsql Meter instead of using EventCounters
eerhardt Nov 1, 2023
43f25f0
Update Telemetry doc for updated Metrics
eerhardt Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...spire.Microsoft.EntityFrameworkCore.SqlServer/AspireSqlServerEFCoreSqlClientExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,15 @@ public static class AspireSqlServerEFCoreSqlClientExtensions
/// <param name="builder">The <see cref="IHostApplicationBuilder" /> to read config from and add services to.</param>
/// <param name="connectionName">A name used to retrieve the connection string from the ConnectionStrings configuration section.</param>
/// <param name="configureSettings">An optional delegate that can be used for customizing options. It's invoked after the settings are read from the configuration.</param>
/// <param name="configureDbContextOptions">An optional delegate to configure the <see cref="DbContextOptions"/> for the context.</param>
/// <remarks>Reads the configuration from "Aspire:Microsoft:EntityFrameworkCore:SqlServer:{typeof(TContext).Name}" config section, or "Aspire:Microsoft:EntityFrameworkCore:SqlServer" if former does not exist.</remarks>
/// <exception cref="ArgumentNullException">Thrown if mandatory <paramref name="builder"/> is null.</exception>
/// <exception cref="InvalidOperationException">Thrown when mandatory <see cref="MicrosoftEntityFrameworkCoreSqlServerSettings.ConnectionString"/> is not provided.</exception>
public static void AddSqlServerDbContext<[DynamicallyAccessedMembers(RequiredByEF)] TContext>(
this IHostApplicationBuilder builder,
string connectionName,
Action<MicrosoftEntityFrameworkCoreSqlServerSettings>? configureSettings = null) where TContext : DbContext
Action<MicrosoftEntityFrameworkCoreSqlServerSettings>? configureSettings = null,
Action<DbContextOptionsBuilder>? configureDbContextOptions = null) where TContext : DbContext
{
ArgumentNullException.ThrowIfNull(builder);

Expand Down Expand Up @@ -115,6 +117,8 @@ void ConfigureDbContext(DbContextOptionsBuilder dbContextOptionsBuilder)
builder.CommandTimeout(settings.Timeout);
}
});

configureDbContextOptions?.Invoke(dbContextOptionsBuilder);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually right? Invoking this after the call to UseSqlServer seems wrong doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The users options/intentions should win over our default implementation. They always get the last say. So we invoke them at the end, after we are done configuring the options, so their options win.

Note that if we touch 2 properties, and they touch 2 other properties, all values "merge" together.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But EF has already looked at the options by the time you use it here. Also note that in your test, you end up with two calls to UseSqlServer one in the extension method, and one in your lambda to get to the SqlServerDbContextOptionsBuilder which doesn't seem right either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But EF has already looked at the options by the time you use it here.

That isn't correct. EF won't look at the options until we return from our ConfigureDbContext method here. So the caller can still modify the options.

Also note that in your test, you end up with two calls to UseSqlServer one in the extension method, and one in your lambda to get to the SqlServerDbContextOptionsBuilder which doesn't seem right either.

But it works - as the test shows. I believe this is expected behavior, you can call UseXXX multiple times on the same DbContextOptionsBuilder instance and the mutations aggregate with the last one winning. That's at least what @roji told me this morning.

The alternative would be for Aspire to take 2 actions - one for the DbContextOptions and another for the Db specific options - ex. SqlServerDbContextOptionsBuilder. This doesn't seem desirable and is probably confusing.

cc @ajcvickers @bricelam

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works in this case, but its a) ugly and b) there is no guarantee that executing the delegate twice for a given context instance will work. It depends what code the user is running in the delegate.

Maybe what Shay was referring to is that both the delegate that builds the options for D.I. and the override of OnConfiguring can be used together. This is useful when some options are set using D.I. and some options are always set on the context and don't change based on D.I.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's the recommendation here? If users need to be able to configure both, what should Aspire do?

cc @davidfowl

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I have not been included in any of the Aspire work, so I don't have any answer for that.

Copy link
Member Author

@eerhardt eerhardt Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this me including you in Aspire work. 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to file an issue on EF about DbContextOptions<T> not being composable (it doesn't use options under the covers) but I was waiting to see how this was going to pan out.

}
}
}
20 changes: 12 additions & 8 deletions src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public static partial class AspireEFPostgreSqlExtensions
/// <param name="builder">The <see cref="IHostApplicationBuilder" /> to read config from and add services to.</param>
/// <param name="connectionName">A name used to retrieve the connection string from the ConnectionStrings configuration section.</param>
/// <param name="configureSettings">An optional delegate that can be used for customizing options. It's invoked after the settings are read from the configuration.</param>
/// <param name="configureDbContextOptions">An optional delegate to configure the <see cref="DbContextOptions"/> for the context.</param>
/// <remarks>
/// <para>
/// Reads the configuration from "Aspire:Npgsql:EntityFrameworkCore:PostgreSQL:{typeof(TContext).Name}" config section, or "Aspire:Npgsql:EntityFrameworkCore:PostgreSQL" if former does not exist.
Expand All @@ -44,7 +45,8 @@ public static partial class AspireEFPostgreSqlExtensions
public static void AddNpgsqlDbContext<[DynamicallyAccessedMembers(RequiredByEF)] TContext>(
this IHostApplicationBuilder builder,
string connectionName,
Action<NpgsqlEntityFrameworkCorePostgreSQLSettings>? configureSettings = null) where TContext : DbContext
Action<NpgsqlEntityFrameworkCorePostgreSQLSettings>? configureSettings = null,
Action<DbContextOptionsBuilder>? configureDbContextOptions = null) where TContext : DbContext
{
ArgumentNullException.ThrowIfNull(builder);

Expand Down Expand Up @@ -111,20 +113,20 @@ public static partial class AspireEFPostgreSqlExtensions
builder.Services.AddOpenTelemetry()
.WithMetrics(meterProviderBuilder =>
{
// Currently both EF and Npgsql provide only Event Counters:
// https://www.npgsql.org/doc/diagnostics/metrics.html?q=metrics
// Currently EF provides only Event Counters:
// https://learn.microsoft.com/en-us/ef/core/logging-events-diagnostics/event-counters?tabs=windows#counters-and-their-meaning
meterProviderBuilder.AddEventCountersInstrumentation(eventCountersInstrumentationOptions =>
{
// The magic strings come from:
// https://github.com/npgsql/npgsql/blob/b3282aa6124184162b66dd4ab828041f872bc602/src/Npgsql/NpgsqlEventSource.cs#L14
// https://github.com/dotnet/efcore/blob/a1cd4f45aa18314bc91d2b9ea1f71a3b7d5bf636/src/EFCore/Infrastructure/EntityFrameworkEventSource.cs#L45
eventCountersInstrumentationOptions.AddEventSources("Npgsql", "Microsoft.EntityFrameworkCore");
// not adding Npgsql.Sql here, as it's used only for Command Start&Stop events
eventCountersInstrumentationOptions.AddEventSources("Microsoft.EntityFrameworkCore");
});

// Very recently Npgsql implemented the Metrics support: https://github.com/npgsql/npgsql/pull/5158
// Currently it's not available at nuget.org, we need to wait.
// https://github.com/npgsql/npgsql/blob/4c9921de2dfb48fb5a488787fc7422add3553f50/src/Npgsql/MetricsReporter.cs#L48
meterProviderBuilder.AddMeter("Npgsql");

// disable "prepared_ratio" until https://github.com/dotnet/aspire/issues/629 is fixed.
meterProviderBuilder.AddView(instrumentName: "db.client.commands.prepared_ratio", MetricStreamConfiguration.Drop);
});
}

Expand All @@ -148,6 +150,8 @@ void ConfigureDbContext(DbContextOptionsBuilder dbContextOptionsBuilder)
// https://www.npgsql.org/doc/connection-string-parameters.html#timeouts-and-keepalive
// There is nothing for us to set here.
});

configureDbContextOptions?.Invoke(dbContextOptionsBuilder);
}
}
}
1 change: 0 additions & 1 deletion src/Components/Aspire.Npgsql/Aspire.Npgsql.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
<PackageReference Include="Npgsql.DependencyInjection" />
<PackageReference Include="Npgsql.OpenTelemetry" />
<PackageReference Include="OpenTelemetry.Extensions.Hosting" />
<PackageReference Include="OpenTelemetry.Instrumentation.EventCounters" />
</ItemGroup>

</Project>
11 changes: 5 additions & 6 deletions src/Components/Aspire.Npgsql/AspirePostgreSqlNpgsqlExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,11 @@ private static void AddNpgsqlDataSource(IHostApplicationBuilder builder, string
builder.Services.AddOpenTelemetry()
.WithMetrics(meterProviderBuilder =>
{
// https://www.npgsql.org/doc/diagnostics/metrics.html?q=metrics
meterProviderBuilder.AddEventCountersInstrumentation(eventCountersInstrumentationOptions =>
{
// https://github.com/npgsql/npgsql/blob/b3282aa6124184162b66dd4ab828041f872bc602/src/Npgsql/NpgsqlEventSource.cs#L14
eventCountersInstrumentationOptions.AddEventSources("Npgsql");
});
// https://github.com/npgsql/npgsql/blob/4c9921de2dfb48fb5a488787fc7422add3553f50/src/Npgsql/MetricsReporter.cs#L48
meterProviderBuilder.AddMeter("Npgsql");

// disable "prepared_ratio" until https://github.com/dotnet/aspire/issues/629 is fixed.
meterProviderBuilder.AddView(instrumentName: "db.client.commands.prepared_ratio", MetricStreamConfiguration.Drop);
});
}
}
Expand Down
40 changes: 20 additions & 20 deletions src/Components/Telemetry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,16 +110,16 @@ Aspire.Npgsql:
- "Npgsql"
- Metric names:
- "Npgsql":
- "ec_Npgsql_bytes_written_per_second"
- "ec_Npgsql_bytes_read_per_second"
- "ec_Npgsql_commands_per_second"
- "ec_Npgsql_total_commands"
- "ec_Npgsql_current_commands"
- "ec_Npgsql_failed_commands"
- "ec_Npgsql_prepared_commands_ratio"
- "ec_Npgsql_connection_pools"
- "ec_Npgsql_multiplexing_average_commands_per_batch"
- "ec_Npgsql_multiplexing_average_write_time_per_batch"
- "db.client.commands.bytes_read"
- "db.client.commands.bytes_written"
- "db.client.commands.duration"
- "db.client.commands.executing"
- "db.client.commands.failed"
- "db.client.connections.create_time"
- "db.client.connections.max"
- "db.client.connections.pending_requests"
- "db.client.connections.timeouts"
- "db.client.connections.usage"

Aspire.Npgsql.EntityFrameworkCore.PostgreSQL:
- Log categories:
Expand Down Expand Up @@ -148,16 +148,16 @@ Aspire.Npgsql.EntityFrameworkCore.PostgreSQL:
- "ec_Microsoft_EntityFramew_total_optimistic_concurrency_failures"
- "ec_Microsoft_EntityF_optimistic_concurrency_failures_per_second"
- "Npgsql":
- "ec_Npgsql_bytes_written_per_second"
- "ec_Npgsql_bytes_read_per_second"
- "ec_Npgsql_commands_per_second"
- "ec_Npgsql_total_commands"
- "ec_Npgsql_current_commands"
- "ec_Npgsql_failed_commands"
- "ec_Npgsql_prepared_commands_ratio"
- "ec_Npgsql_connection_pools"
- "ec_Npgsql_multiplexing_average_commands_per_batch"
- "ec_Npgsql_multiplexing_average_write_time_per_batch"
- "db.client.commands.bytes_read"
- "db.client.commands.bytes_written"
- "db.client.commands.duration"
- "db.client.commands.executing"
- "db.client.commands.failed"
- "db.client.connections.create_time"
- "db.client.connections.max"
- "db.client.connections.pending_requests"
- "db.client.connections.timeouts"
- "db.client.connections.usage"

Aspire.RabbitMQ.Client:
- Log categories:
Expand Down
48 changes: 48 additions & 0 deletions ...soft.EntityFrameworkCore.SqlServer.Tests/AspireSqlServerEFCoreSqlClientExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@

using Aspire.Components.Common.Tests;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Infrastructure.Internal;
using Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal;
using Microsoft.EntityFrameworkCore.Storage;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
Expand Down Expand Up @@ -68,4 +71,49 @@ public void ConnectionNameWinsOverConfigSection()
// the connection string from config should not be used since it was found in ConnectionStrings
Assert.DoesNotContain("unused", actualConnectionString);
}

[Fact]
public void CanConfigureDbContextOptions()
{
var builder = Host.CreateEmptyApplicationBuilder(null);
builder.Configuration.AddInMemoryCollection([
new KeyValuePair<string, string?>("ConnectionStrings:sqlconnection", ConnectionString),
new KeyValuePair<string, string?>("Aspire:Microsoft:EntityFrameworkCore:SqlServer:MaxRetryCount", "304"),
new KeyValuePair<string, string?>("Aspire:Microsoft:EntityFrameworkCore:SqlServer:Timeout", "608")
]);

builder.AddSqlServerDbContext<TestDbContext>("sqlconnection", configureDbContextOptions: optionsBuilder =>
{
optionsBuilder.UseSqlServer(sqlBuilder =>
{
sqlBuilder.MinBatchSize(123);
});
});

var host = builder.Build();
var context = host.Services.GetRequiredService<TestDbContext>();

#pragma warning disable EF1001 // Internal EF Core API usage.

var extension = context.Options.FindExtension<SqlServerOptionsExtension>();
Assert.NotNull(extension);

// ensure the min batch size was respected
Assert.Equal(123, extension.MinBatchSize);

// ensure the connection string from config was respected
var actualConnectionString = context.Database.GetDbConnection().ConnectionString;
Assert.Equal(ConnectionString, actualConnectionString);

// ensure the max retry count from config was respected
Assert.NotNull(extension.ExecutionStrategyFactory);
var executionStrategy = extension.ExecutionStrategyFactory(new ExecutionStrategyDependencies(new CurrentDbContext(context), context.Options, null!));
var retryStrategy = Assert.IsType<SqlServerRetryingExecutionStrategy>(executionStrategy);
Assert.Equal(304, retryStrategy.MaxRetryCount);

// ensure the command timeout from config was respected
Assert.Equal(608, extension.CommandTimeout);

#pragma warning restore EF1001 // Internal EF Core API usage.
}
}
45 changes: 45 additions & 0 deletions ...s/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/AspireEFPostgreSqlExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@

using Aspire.Components.Common.Tests;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Infrastructure.Internal;
using Microsoft.EntityFrameworkCore.Storage;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Npgsql.EntityFrameworkCore.PostgreSQL;
using Npgsql.EntityFrameworkCore.PostgreSQL.Infrastructure.Internal;
using Xunit;

namespace Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests;
Expand Down Expand Up @@ -68,4 +72,45 @@ public void ConnectionNameWinsOverConfigSection()
// the connection string from config should not be used since it was found in ConnectionStrings
Assert.DoesNotContain("unused", actualConnectionString);
}

[Fact]
public void CanConfigureDbContextOptions()
{
var builder = Host.CreateEmptyApplicationBuilder(null);
builder.Configuration.AddInMemoryCollection([
new KeyValuePair<string, string?>("ConnectionStrings:npgsql", ConnectionString),
new KeyValuePair<string, string?>("Aspire:Npgsql:EntityFrameworkCore:PostgreSQL:MaxRetryCount", "304")
]);

builder.AddNpgsqlDbContext<TestDbContext>("npgsql", configureDbContextOptions: optionsBuilder =>
{
optionsBuilder.UseNpgsql(npgsqlBuilder =>
{
npgsqlBuilder.CommandTimeout(123);
});
});

var host = builder.Build();
var context = host.Services.GetRequiredService<TestDbContext>();

#pragma warning disable EF1001 // Internal EF Core API usage.

var extension = context.Options.FindExtension<NpgsqlOptionsExtension>();
Assert.NotNull(extension);

// ensure the command timeout was respected
Assert.Equal(123, extension.CommandTimeout);

// ensure the connection string from config was respected
var actualConnectionString = context.Database.GetDbConnection().ConnectionString;
Assert.Equal(ConnectionString, actualConnectionString);

// ensure the max retry count from config was respected
Assert.NotNull(extension.ExecutionStrategyFactory);
var executionStrategy = extension.ExecutionStrategyFactory(new ExecutionStrategyDependencies(new CurrentDbContext(context), context.Options, null!));
var retryStrategy = Assert.IsType<NpgsqlRetryingExecutionStrategy>(executionStrategy);
Assert.Equal(304, retryStrategy.MaxRetryCount);

#pragma warning restore EF1001 // Internal EF Core API usage.
}
}
3 changes: 3 additions & 0 deletions tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/TestDbContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ public class TestDbContext : DbContext
{
public TestDbContext(DbContextOptions<TestDbContext> options) : base(options)
{
Options = options;
}

public DbContextOptions<TestDbContext> Options { get; }

public DbSet<CatalogBrand> CatalogBrands => Set<CatalogBrand>();

public class CatalogBrand
Expand Down