feat(aspire): add .NET Aspire orchestration and observability#9
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces .NET Aspire orchestration to the Carpentry Shop project, adding a distributed application host ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppHost as AppHost<br/>(DistributedApplication)
participant PostgreSQL as PostgreSQL<br/>Container
participant WebApp as Web Application
participant DB as DbContext
participant OTel as OpenTelemetry<br/>& Health Checks
User->>AppHost: dotnet run / aspire run
AppHost->>PostgreSQL: Provision container (image:17)
PostgreSQL-->>AppHost: Container started
AppHost->>PostgreSQL: Create 'carpentrydb' database
PostgreSQL-->>AppHost: Database ready
AppHost->>WebApp: Register service<br/>(with carpentrydb ref)
WebApp->>WebApp: AddServiceDefaults()
WebApp->>OTel: Configure OpenTelemetry<br/>(logging, metrics, tracing)
WebApp->>OTel: Setup health checks<br/>(/health, /alive endpoints)
WebApp->>DB: WaitFor DB readiness
PostgreSQL-->>DB: Connection established
DB-->>WebApp: DbContext ready
AppHost->>AppHost: Build DistributedApplication
AppHost-->>User: Running (dashboard at endpoint)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
CarpentryShop.ServiceDefaults/CarpentryShop.ServiceDefaults.csproj (1)
13-17: Minor OpenTelemetry version variance.The OpenTelemetry packages use slightly different versions (
1.11.2for core/hosting vs1.11.1for instrumentation). This is common as these packages release independently, but you may want to align them when all packages have a matching version available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CarpentryShop.ServiceDefaults/CarpentryShop.ServiceDefaults.csproj` around lines 13 - 17, Some OpenTelemetry PackageReference entries are using mismatched versions (OpenTelemetry.Exporter.OpenTelemetryProtocol and OpenTelemetry.Extensions.Hosting at 1.11.2 vs OpenTelemetry.Instrumentation.AspNetCore, OpenTelemetry.Instrumentation.Http, OpenTelemetry.Instrumentation.Runtime at 1.11.1); update the PackageReference versions so all OpenTelemetry packages (e.g., OpenTelemetry.Exporter.OpenTelemetryProtocol, OpenTelemetry.Extensions.Hosting, OpenTelemetry.Instrumentation.AspNetCore, OpenTelemetry.Instrumentation.Http, OpenTelemetry.Instrumentation.Runtime) use the same compatible version (pick the latest common version) to keep dependency alignment and avoid runtime mismatches.CarpentryShop.AppHost/Program.cs (1)
4-7: Consider adding data persistence for PostgreSQL.The PostgreSQL container will lose data when stopped. For development scenarios where data persistence across restarts is desired, consider adding a volume:
💡 Optional: Add data persistence
var postgres = builder.AddPostgres("postgres") - .WithImageTag("17"); + .WithImageTag("17") + .WithDataVolume();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CarpentryShop.AppHost/Program.cs` around lines 4 - 7, The Postgres container created via builder.AddPostgres("postgres") (and configured with .WithImageTag("17") and postgres.AddDatabase("carpentrydb")) has no persisted data; add a persistent volume or bind-mount for the Postgres data directory (e.g. /var/lib/postgresql/data) so DB files survive container restarts. Update the Postgres setup (the object returned by builder.AddPostgres / the postgres variable) to attach a named volume or host bind (e.g. .WithVolume("pgdata", "/var/lib/postgresql/data") or equivalent in your test container API) before calling AddDatabase to ensure carpentryDb data is persisted across restarts.CarpentryShop.csproj (1)
22-22: Aspire package version inconsistency.
Aspire.Npgsql.EntityFrameworkCore.PostgreSQLis version13.2.0here, whileCarpentryShop.AppHost.csprojuses13.1.2for the hosting packages. Consider aligning all Aspire-related packages to the same version to avoid potential compatibility issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CarpentryShop.csproj` at line 22, The Aspire package version is inconsistent: update the PackageReference for Aspire.Npgsql.EntityFrameworkCore.PostgreSQL in CarpentryShop.csproj to match the Aspire hosting packages used in CarpentryShop.AppHost.csproj (use the same version, e.g., change 13.2.0 to 13.1.2), or alternatively update all Aspire package references across both projects to a single chosen version; ensure the PackageReference element for Aspire.Npgsql.EntityFrameworkCore.PostgreSQL and any other Aspire.* PackageReference entries share the exact same Version value.justfile (2)
81-83: Security note: curl-pipe-to-bash pattern.While
curl | bashis a common installation pattern for developer tools, it executes remote code without inspection. This is acceptable for documented CLI tools like Aspire, but consider noting the trade-off in project documentation or recommending developers inspect the script first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 81 - 83, The install-aspire recipe uses the unsafe curl | bash pattern; change the recipe or docs to avoid blindly executing remote scripts: update the install-aspire target to first download the installer (e.g., with curl -sSL -o aspire-install.sh), optionally verify its contents/signature, and then run it (sh aspire-install.sh) or add a comment and README note advising developers to inspect the script before running; reference the install-aspire recipe name when making this change so the safer download-and-execute or documentation warning replaces the direct curl | bash command.
22-24: Consider adding Aspire watch support.The
watchcommand runs the main project directly without Aspire orchestration. This is fine for UI/code changes, but developers won't have access to the Aspire-managed PostgreSQL container during hot reload.You might want to document this limitation or add a separate
watch-aspiretask if Aspire supports watch mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 22 - 24, The current justfile target "watch" runs dotnet watch directly and thus does not start Aspire-managed services (e.g., the PostgreSQL container), so hot-reload runs without Aspire orchestration; add either a new target named "watch-aspire" that brings up Aspire services before running the same dotnet watch command (or runs dotnet watch through Aspire), or update the project README to document that "watch" does not provide Aspire services and recommend using the new "watch-aspire" task for database-dependent development. Ensure references to the just target "watch" and the new "watch-aspire" (or README section) are clear so developers know which to use.CarpentryShop.ServiceDefaults/Extensions.cs (1)
69-75: Consider adding database health check.The current "self" health check only reports that the application is running. For a more comprehensive health status, consider adding a database connectivity check since the application depends on PostgreSQL.
💡 Optional enhancement
public static IHostApplicationBuilder AddDefaultHealthChecks(this IHostApplicationBuilder builder) { builder.Services.AddHealthChecks() - .AddCheck("self", () => HealthCheckResult.Healthy(), ["live"]); + .AddCheck("self", () => HealthCheckResult.Healthy(), ["live"]) + .AddNpgSql(name: "postgres", tags: ["ready"]); return builder; }This requires the
AspNetCore.HealthChecks.NpgSqlpackage and would need the connection string to be available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CarpentryShop.ServiceDefaults/Extensions.cs` around lines 69 - 75, Update AddDefaultHealthChecks to register a PostgreSQL health check in addition to the existing "self" check: install/ensure AspNetCore.HealthChecks.NpgSql is available, read the Postgres connection string from configuration (e.g., IConfiguration or builder.Configuration) and add .AddNpgSql(connectionString, name: "postgres", tags: new[] { "db", "ready" }) to the builder.Services.AddHealthChecks() chain so the method registers both the "self" liveness check and a DB connectivity readiness check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CarpentryShop.ServiceDefaults/Extensions.cs`:
- Around line 77-89: MapDefaultEndpoints currently registers /health and /alive
only when app.Environment.IsDevelopment(), which means production builds lack
health endpoints; either document this intentional omission or change
MapDefaultEndpoints to always map a minimal /health endpoint (keep the existing
/alive mapping conditional if desired) so orchestrators can probe; update the
method MapDefaultEndpoints to unconditionally call
app.MapHealthChecks("/health") and retain app.MapHealthChecks("/alive", new
HealthCheckOptions { Predicate = r => r.Tags.Contains("live") }) inside the
Development branch, and add a short comment explaining the behavior if you
choose to keep /alive dev-only.
In `@Program.cs`:
- Around line 114-115: The MapDefaultEndpoints() call currently only registers
/health and /alive when IsDevelopment() is true (see MapDefaultEndpoints() in
Extensions.cs), so those endpoints are unavailable in non-development
deployments; update Extensions.cs to register the /health and /alive endpoints
unconditionally (or add a configuration flag to enable them in Production) by
removing or altering the IsDevelopment() guard inside MapDefaultEndpoints() so
that the health endpoints are always mapped (or controlled via configuration)
and thus available to docker/production probes.
---
Nitpick comments:
In `@CarpentryShop.AppHost/Program.cs`:
- Around line 4-7: The Postgres container created via
builder.AddPostgres("postgres") (and configured with .WithImageTag("17") and
postgres.AddDatabase("carpentrydb")) has no persisted data; add a persistent
volume or bind-mount for the Postgres data directory (e.g.
/var/lib/postgresql/data) so DB files survive container restarts. Update the
Postgres setup (the object returned by builder.AddPostgres / the postgres
variable) to attach a named volume or host bind (e.g. .WithVolume("pgdata",
"/var/lib/postgresql/data") or equivalent in your test container API) before
calling AddDatabase to ensure carpentryDb data is persisted across restarts.
In `@CarpentryShop.csproj`:
- Line 22: The Aspire package version is inconsistent: update the
PackageReference for Aspire.Npgsql.EntityFrameworkCore.PostgreSQL in
CarpentryShop.csproj to match the Aspire hosting packages used in
CarpentryShop.AppHost.csproj (use the same version, e.g., change 13.2.0 to
13.1.2), or alternatively update all Aspire package references across both
projects to a single chosen version; ensure the PackageReference element for
Aspire.Npgsql.EntityFrameworkCore.PostgreSQL and any other Aspire.*
PackageReference entries share the exact same Version value.
In `@CarpentryShop.ServiceDefaults/CarpentryShop.ServiceDefaults.csproj`:
- Around line 13-17: Some OpenTelemetry PackageReference entries are using
mismatched versions (OpenTelemetry.Exporter.OpenTelemetryProtocol and
OpenTelemetry.Extensions.Hosting at 1.11.2 vs
OpenTelemetry.Instrumentation.AspNetCore, OpenTelemetry.Instrumentation.Http,
OpenTelemetry.Instrumentation.Runtime at 1.11.1); update the PackageReference
versions so all OpenTelemetry packages (e.g.,
OpenTelemetry.Exporter.OpenTelemetryProtocol, OpenTelemetry.Extensions.Hosting,
OpenTelemetry.Instrumentation.AspNetCore, OpenTelemetry.Instrumentation.Http,
OpenTelemetry.Instrumentation.Runtime) use the same compatible version (pick the
latest common version) to keep dependency alignment and avoid runtime
mismatches.
In `@CarpentryShop.ServiceDefaults/Extensions.cs`:
- Around line 69-75: Update AddDefaultHealthChecks to register a PostgreSQL
health check in addition to the existing "self" check: install/ensure
AspNetCore.HealthChecks.NpgSql is available, read the Postgres connection string
from configuration (e.g., IConfiguration or builder.Configuration) and add
.AddNpgSql(connectionString, name: "postgres", tags: new[] { "db", "ready" }) to
the builder.Services.AddHealthChecks() chain so the method registers both the
"self" liveness check and a DB connectivity readiness check.
In `@justfile`:
- Around line 81-83: The install-aspire recipe uses the unsafe curl | bash
pattern; change the recipe or docs to avoid blindly executing remote scripts:
update the install-aspire target to first download the installer (e.g., with
curl -sSL -o aspire-install.sh), optionally verify its contents/signature, and
then run it (sh aspire-install.sh) or add a comment and README note advising
developers to inspect the script before running; reference the install-aspire
recipe name when making this change so the safer download-and-execute or
documentation warning replaces the direct curl | bash command.
- Around line 22-24: The current justfile target "watch" runs dotnet watch
directly and thus does not start Aspire-managed services (e.g., the PostgreSQL
container), so hot-reload runs without Aspire orchestration; add either a new
target named "watch-aspire" that brings up Aspire services before running the
same dotnet watch command (or runs dotnet watch through Aspire), or update the
project README to document that "watch" does not provide Aspire services and
recommend using the new "watch-aspire" task for database-dependent development.
Ensure references to the just target "watch" and the new "watch-aspire" (or
README section) are clear so developers know which to use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 220ccb01-fac6-4b52-972a-8e3547a7e024
📒 Files selected for processing (11)
CarpentryShop.AppHost/CarpentryShop.AppHost.csprojCarpentryShop.AppHost/Program.csCarpentryShop.AppHost/Properties/launchSettings.jsonCarpentryShop.ServiceDefaults/CarpentryShop.ServiceDefaults.csprojCarpentryShop.ServiceDefaults/Extensions.csCarpentryShop.csprojCarpentryShop.slnProgram.csappsettings.Development.jsonaspire.config.jsonjustfile
Comment on lines
+77
to
+89
| public static WebApplication MapDefaultEndpoints(this WebApplication app) | ||
| { | ||
| if (app.Environment.IsDevelopment()) | ||
| { | ||
| app.MapHealthChecks("/health"); | ||
| app.MapHealthChecks("/alive", new HealthCheckOptions | ||
| { | ||
| Predicate = r => r.Tags.Contains("live") | ||
| }); | ||
| } | ||
|
|
||
| return app; | ||
| } |
There was a problem hiding this comment.
Production deployments will lack health check endpoints.
The /health and /alive endpoints are only mapped in Development mode. This is a deliberate choice but has implications:
- Container orchestrators (Kubernetes, ECS) won't be able to use these endpoints for liveness/readiness probes
- Load balancers won't have health check targets
- Docker-compose deployments (per
docker-compose.ymlcontext) run without these endpoints
If this is intentional (e.g., using a separate health check mechanism in production), document this decision. Otherwise, consider mapping at least a basic /health endpoint unconditionally.
💡 Suggested change for production health checks
public static WebApplication MapDefaultEndpoints(this WebApplication app)
{
+ // Always expose basic health check for orchestrators
+ app.MapHealthChecks("/health");
+
if (app.Environment.IsDevelopment())
{
- app.MapHealthChecks("/health");
app.MapHealthChecks("/alive", new HealthCheckOptions
{
Predicate = r => r.Tags.Contains("live")
});
}
return app;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static WebApplication MapDefaultEndpoints(this WebApplication app) | |
| { | |
| if (app.Environment.IsDevelopment()) | |
| { | |
| app.MapHealthChecks("/health"); | |
| app.MapHealthChecks("/alive", new HealthCheckOptions | |
| { | |
| Predicate = r => r.Tags.Contains("live") | |
| }); | |
| } | |
| return app; | |
| } | |
| public static WebApplication MapDefaultEndpoints(this WebApplication app) | |
| { | |
| // Always expose basic health check for orchestrators | |
| app.MapHealthChecks("/health"); | |
| if (app.Environment.IsDevelopment()) | |
| { | |
| app.MapHealthChecks("/alive", new HealthCheckOptions | |
| { | |
| Predicate = r => r.Tags.Contains("live") | |
| }); | |
| } | |
| return app; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CarpentryShop.ServiceDefaults/Extensions.cs` around lines 77 - 89,
MapDefaultEndpoints currently registers /health and /alive only when
app.Environment.IsDevelopment(), which means production builds lack health
endpoints; either document this intentional omission or change
MapDefaultEndpoints to always map a minimal /health endpoint (keep the existing
/alive mapping conditional if desired) so orchestrators can probe; update the
method MapDefaultEndpoints to unconditionally call
app.MapHealthChecks("/health") and retain app.MapHealthChecks("/alive", new
HealthCheckOptions { Predicate = r => r.Tags.Contains("live") }) inside the
Development branch, and add a short comment explaining the behavior if you
choose to keep /alive dev-only.
Comment on lines
+114
to
+115
| // Map Aspire default endpoints | ||
| app.MapDefaultEndpoints(); |
There was a problem hiding this comment.
Health endpoints are only available in Development mode.
Per the MapDefaultEndpoints() implementation in Extensions.cs (lines 77-89), the /health and /alive endpoints are only mapped when IsDevelopment() returns true. This means:
- Docker-compose deployments (typically running in Production/Staging mode) won't have these health endpoints available.
- If you need health checks for container orchestration or load balancers in non-development environments, you'll need to either:
- Map them unconditionally
- Use a different mechanism for production health probes
Consider whether this is intentional for your deployment strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Program.cs` around lines 114 - 115, The MapDefaultEndpoints() call currently
only registers /health and /alive when IsDevelopment() is true (see
MapDefaultEndpoints() in Extensions.cs), so those endpoints are unavailable in
non-development deployments; update Extensions.cs to register the /health and
/alive endpoints unconditionally (or add a configuration flag to enable them in
Production) by removing or altering the IsDevelopment() guard inside
MapDefaultEndpoints() so that the health endpoints are always mapped (or
controlled via configuration) and thus available to docker/production probes.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 20466352 | Triggered | Generic Password | 8d9115f | appsettings.Development.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit
New Features
Chores