Feature/aws v4 cloudwatch publisher - #20
Conversation
…com/robvangeloven/HealthChecks into feature/aws-v4-cloudwatch-publisher
There was a problem hiding this comment.
Pull request overview
Fixes the CloudWatch health check publisher to be compatible with AWSSDK.CloudWatch v4 serialization rules (issue #18), and updates tests/samples to newer ASP.NET Core hosting patterns while also pinning OpenTelemetry.Api to address dependency/security warnings.
Changes:
- Remove
StatisticValuesinitialization from CloudWatchMetricDatumwhenValueis used, avoiding AWS’ “mutually exclusive” parameter error. - Update shared conformance test hosting to
WebApplication/TestServer. - Migrate multiple sample apps from
Startup/WebHostto minimal hosting (Program.cs) and add an explicitOpenTelemetry.Apipin/reference.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/_SHARED/TestHostHelper.cs | Minor formatting/consistency update in shared test host helper. |
| test/_SHARED/ConformanceTests.cs | Switch test hosting from HostBuilder/ConfigureWebHost to WebApplication + UseTestServer. |
| test/HealthChecks.Publisher.ApplicationInsights.Tests/HealthChecks.Publisher.ApplicationInsights.Tests.csproj | Add direct OpenTelemetry.Api reference (central version managed). |
| src/HealthChecks.System/DependencyInjection/SystemHealthCheckBuilderExtensions.cs | Whitespace/formatting-only adjustment. |
| src/HealthChecks.Publisher.CloudWatch/CloudWatchPublisher.cs | Stop sending StatisticValues alongside Value in CloudWatch metrics (core fix for #18). |
| samples/HealthChecks.UIAndApiCustomization/Startup.cs | Remove legacy Startup in favor of minimal hosting. |
| samples/HealthChecks.UIAndApiCustomization/Program.cs | Minimal hosting version of UI+API customization sample. |
| samples/HealthChecks.UIAndApi/Startup.cs | Remove legacy Startup in favor of minimal hosting. |
| samples/HealthChecks.UIAndApi/Program.cs | Minimal hosting version of UI+API sample. |
| samples/HealthChecks.UI.StorageProviders/Startup.cs | Remove legacy Startup in favor of minimal hosting. |
| samples/HealthChecks.UI.StorageProviders/Program.cs | Minimal hosting version of storage providers sample. |
| samples/HealthChecks.UI.Sample/Startup.cs | Remove legacy Startup in favor of minimal hosting. |
| samples/HealthChecks.UI.Sample/Program.cs | Minimal hosting version of UI sample. |
| samples/HealthChecks.UI.Oidc/Startup.cs | Remove legacy Startup in favor of minimal hosting. |
| samples/HealthChecks.UI.Oidc/Program.cs | Minimal hosting version of OIDC-protected UI sample. |
| samples/HealthChecks.UI.Branding/Startup.cs | Remove legacy Startup in favor of minimal hosting. |
| samples/HealthChecks.UI.Branding/Program.cs | Minimal hosting version of branding sample. |
| samples/HealthChecks.Sample/Startup.cs | Remove legacy Startup in favor of minimal hosting. |
| samples/HealthChecks.Sample/Program.cs | Minimal hosting version of general health checks sample. |
| samples/HealthChecks.Sample/HealthChecks.Sample.csproj | Add direct OpenTelemetry.Api reference (central version managed). |
| Directory.Packages.props | Central package version pin for OpenTelemetry.Api. |
Comments suppressed due to low confidence (1)
src/HealthChecks.Publisher.CloudWatch/CloudWatchPublisher.cs:55
- Add a regression test for the AWS SDK v4 serialization behavior this change is addressing (MetricDatum must not include StatisticValues when Value is set). Without a test, it’s easy for a future refactor to reintroduce the InvalidParameterCombinationException that prompted issue #18.
new MetricDatum
{
Dimensions = _dimensions,
MetricName = "status",
Timestamp = utcNow,
Unit = StandardUnit.Count,
Value = (int)report.Status
}
| WebHost.CreateDefaultBuilder(args) | ||
| .UseStartup<Startup>(); | ||
| } | ||
| app.UseRouting(); |
There was a problem hiding this comment.
In the minimal hosting model, calling UseRouting() without also calling UseEndpoints(...) can result in endpoints (including MapHealthChecksUI) not being executed / throwing at runtime. Either remove UseRouting() (since MapHealthChecksUI works without it here) or add UseEndpoints and map the UI endpoints inside it.
| app.UseRouting(); |
|
@copilot apply changes based on the comments in this thread |
|
@copilot HealthChecks System CI / build / build (pull_request)Failing after 35s |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
What this PR does / why we need it:
Fixes the #18 issue
Which issue(s) this PR fixes:
Please reference the issue this PR will close: #18
Special notes for your reviewer:
I needed to fix a lot of errors locally, security warning on open telemetry package and deprecation of IWebHost in dotnet10. However I couldn't get some tests to run properly locally
Does this PR introduce a user-facing change?: No
Please make sure you've completed the relevant tasks for this PR, out of the following list: