Aspire Integration#6775
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sbrown-livefront
force-pushed
the
billing/aspire
branch
from
915b6d3 to
f7a8070
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6775 +/- ##
=======================================
Coverage 59.26% 59.26%
=======================================
Files 2082 2082
Lines 92060 92060
Branches 8181 8181
=======================================
Hits 54557 54557
Misses 35562 35562
Partials 1941 1941 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
@sbrown-livefront is there a reason this keeps being synced? I think we can leave it for a moment and wait until .NET 10 gets applied to the platform. |
Sorry about the noise. I had it auto-sync when merging main in locally to make sure it stayed up-to-date. I'll shut that down. |
| builder.Logging.AddOpenTelemetry(logging => | ||
| { | ||
| logging.IncludeFormattedMessage = true; | ||
| logging.IncludeScopes = true; | ||
| }); |
There was a problem hiding this comment.
This method should be able to become just this. The Server SDK does most of below and more. It doesn't filter out health endpoints from instrumentation but it will likely be adding that soon.
There was a problem hiding this comment.
🎉 Great work on the SDK. The entire servicedefaults wasn't needed anymore!
I had some request/response pieces that enriched the httpclient, but they can be added by devs if needed.
It looked like:
{
httpClient.EnrichWithHttpRequestMessage = (activity, message) =>
{
if (context.HostingEnvironment.IsDevelopment())
{
activity.SetTag("http.request_content_length", message.Content?.Headers.ContentLength);
activity.SetTag("http.request_method", message.Method.Method);
activity.SetTag("http.request_url", message.RequestUri?.ToString());
activity.SetTag("http.request_message_headers", message.Headers.ToString());
activity.SetTag("http.request_body", message.Content?.ReadAsStringAsync().Result);
}
};
httpClient.EnrichWithHttpResponseMessage = (activity, message) =>
{
if (context.HostingEnvironment.IsDevelopment())
{
activity.SetTag("http.response_content_length",
message.Content.Headers.ContentLength);
activity.SetTag("http.response_status_code", (int)message.StatusCode);
activity.SetTag("http.response_status_text", message.ReasonPhrase);
activity.SetTag("http.response_content_type",
message.Content.Headers.ContentType?.MediaType);
activity.SetTag("http.response_message_headers", message.Headers.ToString());
activity.SetTag("http.response_body", message.Content.ReadAsStringAsync().Result);
}
};| services.AddServiceDiscovery(); | ||
| services.ConfigureHttpClientDefaults(http => | ||
| { | ||
| http.AddStandardResilienceHandler(); |
There was a problem hiding this comment.
I'm cautious to add AddStandardResilienceHandler() right now. It's likely a good thing to add but I wouldn't want it just added for DEBUG builds. I would want it added to Release as well but we will want to audit the effects it will have first. I don't think it's a requirement for Aspire but correct me if I am wrong.
There was a problem hiding this comment.
Nah, it wasn't required.
Looking at the internals it added some okay defaults for resilience, but I think we'll be fine for it running locally until the SDK includes it.
There was a problem hiding this comment.
I think this is where we should put aspire configuration, for starters this is my thinking of what the layout would be:
{
.. Logging
"Services": {
"admin": {
"BasePort": 1111,
},
.. Other services
}
"AdditionalProjects": {},
"ClientsPath": "../clients",
"SelfHost": false,
"Database": {
"Type": "MsSql",
"Password": "MyPassword"
}
}The idea with this is that AdditionalProjects will start as an empty object but consumers that want to integrate with billing-pricing for example can do:
dotnet user-secrets set "AdditionalProjects:billing-pricing" "../billing-pricing/Path/To/Project.csproj"With base port we can get cute and when they choose self host we just increment the number by one.
There was a problem hiding this comment.
This was a great idea.
I have the service ports empty for now for users to overwrite them.
I also added the more context for the AdditionalProjects. You can add an object like this to make sure it is referenced in the correct projects.
"pricing-service": {
"Path": "../../billing-pricing/src/REST/REST.csproj",
"ReferencedBy": [ "api", "billing" ]
}I'll probably expand it to have projects it want's to reference also, but for now I think it's fine.
|
@justindbaur Sorry for the delay on these fixes. Thank you for the great feedback. That SDK add made everything less intrusive. I added some MSBuild properties for including community plugins I've been using. That way they don't get included if the user doesn't want them. Since they aren't official plugins for Aspire, I didn't want to include them by default. Let me know your thoughts. |
|
4 participants
🎟️ Tracking
📔 Objective
Introduces a new
AppHostproject to the solution, providing a centralized entry point for orchestrating local development and distributed application resources using .NET Aspire. The changes include new configuration files, project setup, and builder extensions to streamline the setup of databases, secrets, mail, storage emulation, and service dependencies. It also adds support for optional community plugins (NodeJS and Ngrok) and dynamic inclusion of additional projects through configuration.The most important changes are:
New AppHost Project Integration:
AppHostproject to the solution, including its project file (AppHost.csproj), main entry point (AppHost.cs), and supporting builder extensions (BuilderExtensions.cs). This project coordinates the setup and orchestration of all major services and infrastructure required for local development. [1] [2] [3] [4] [5]Configuration and Environment Setup:
AppHost, includingappsettings.json,appsettings.Development.json, and launch profiles (launchSettings.json), to define logging, ports, database, mail, storage, and plugin settings. [1] [2] [3].aspire/settings.jsonto specify the path to theAppHostproject for Aspire tooling.Builder Extension Utilities:
BuilderExtensions.csto encapsulate reusable logic for configuring secrets setup, database, migrations, Azurite storage emulator, mail catcher, service wiring, and optional community plugins (NodeJS frontend and Ngrok tunneling). This modularizes and simplifies the orchestration of resources and dependencies.Solution File Updates:
bitwarden-server.slnto include the newAppHostproject and ensure it participates in solution builds and configurations. [1] [2]📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes