-
Notifications
You must be signed in to change notification settings - Fork 685
Filter out tracing health endpoints in service defaults templates #8643
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
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.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
playground/TestShop/TestShop.AppHost/Program.cs:71
- [nitpick] Consider replacing the literal "/health" string with the defined HealthEndpointPath constant to ensure consistency across the codebase.
var _ = frontend.GetEndpoint("https").Exists ? frontend.WithHttpsHealthCheck("/health") : frontend.WithHttpHealthCheck("/health");
/backport to release/9.2 |
Started backporting to release/9.2: https://github.com/dotnet/aspire/actions/runs/14340923842 |
Might still be good to log a docs issue that goes through our code samples in case we have service defaults there to make sure it is updated, and also might be something that it is good to mention in our telemetry section as things are being excluded. Related but separate, we should think about how to mention changes to ServiceDefaults in our what's new as existing apps would not get this change unless they know about it. |
@@ -52,7 +55,12 @@ public static TBuilder ConfigureOpenTelemetry<TBuilder>(this TBuilder builder) w | |||
}) | |||
.WithTracing(tracing => | |||
{ | |||
tracing.AddAspNetCoreInstrumentation() | |||
tracing | |||
.AddAspNetCoreInstrumentation(tracing => |
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.
Do you think it's worth adding a test to playground that validates that health checks are being filtered and not sent through OTLP or is that overkill?
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 that's overkill. We're just using standard APIs from OTel for ASP.NET Core. If we wanted a test it would be on templates I'd think.
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.
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.
Changes LGTM.
FYI logged dotnet/docs-aspire#2939 to track this. |
) * Filter out tracing health endpoints Fixes #8580 * Fix typo
* Migrate from VSTest to Microsoft.Testing.Platform * Address TODO * Cleanup stuff around runsettings * Add comment * Progress * Remove locale * Always show live output * Restore test session timeout * Fix timeout * Fix typo * Fix duplicate commnand-line options * Fix Linux * Fix for Linux * Fix * Fix * Filter failing * Fix * ignore exit code 8 * Fix extra dot * Ignore exit code * Fix duplicate --ignore-exit-code * Fix playground tests on helix * Move to RepoTesting.props * Fix results dir * Fix * Progress * Fix duplicate command-line option * Missing report-trx * Run executable * Address review comments * Revert "Run executable" This reverts commit d10e168. * dotnet dll * Introduce version selector for Aspire templates (#8625) * Introduce version selecto to aspire new * Update src/Aspire.Cli/Commands/NewCommand.cs Co-authored-by: Copilot <[email protected]> * Update src/Aspire.Cli/Commands/NewCommand.cs Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: David Fowler <[email protected]> Co-authored-by: Copilot <[email protected]> * Error handling for GetCapabilitiesAsync connection issues (#8614) * Error out when GetCapabiltiesAsync is missing or unexpected exception occurs. * Update src/Aspire.Cli/DotNetCliRunner.cs Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]> * Obsolete AddAzureContainerAppsInfrastructure (#8639) * Obsolete AddAzureContainerAppsInfrastructure This method is no longer meant to be used. Instead developers should be calling AddAzureContainerAppEnvironment. Covert the tests using this API to the new API * Fix volume output naming issue We weren't discriminating between volumes and bindmounts, which caused a cache collision. * Switch the prefix at the beginning * Add error handling for package updates in workflow (#8390) * Filter out tracing health endpoints in service defaults templates (#8643) * Filter out tracing health endpoints Fixes #8580 * Fix typo * Show neutral icon for container exiting with status code 0 (#8477) * Show neutral icon for container exiting with status code 0 * Show info/stop icons for all exited resources with status code 0 * Add finished test for custom resource, remove exit code expectation * update tests * [tests] Correctly mark Aspire.Dashboard.Tests.Integration.Playwright.AppBarTests with RequiresPlaywright (#8647) * [tests] Skip generating Directory.Packages.Versions.props for Test (#8621) .. Utility projects. * Fix running playground tests on windows --------- Co-authored-by: Ankit Jain <[email protected]> Co-authored-by: Dan Moseley <[email protected]> Co-authored-by: Mitch Denny <[email protected]> Co-authored-by: David Fowler <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Eric Erhardt <[email protected]> Co-authored-by: Jose Perez Rodriguez <[email protected]> Co-authored-by: Damian Edwards <[email protected]> Co-authored-by: Adam Ratzman <[email protected]>
Description
Updates the service defaults templates so that requests to the health endpoints are not traced.
Fixes #8580
Checklist