Update App.Metrics to v4#248
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR upgrades Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Cassandra.IntegrationTests/Cassandra.IntegrationTests.csproj`:
- Line 36: App.Metrics has been bumped to 4.2.0, but the integration tests still
need a full 4.x migration audit across src/Cassandra.IntegrationTests. Review
all App.Metrics usage sites and update them to the 4.x API surface, including
any renamed packages like App.Metrics.AspNetCore, removed Health Checks, and
host model changes from IWebHost to IHost. If the existing metrics integration
cannot be adapted cleanly, replace the App.Metrics references with
OpenTelemetry-based equivalents and remove obsolete App.Metrics dependencies
from the test project.
- Line 36: The integration test project is referencing App.Metrics 4.2.0 while
Cassandra.AppMetrics and AppMetricsGraphite are still pinned to older 3.x
packages, so align the App.Metrics dependency versions across these projects.
Update Cassandra.AppMetrics to use App.Metrics.Abstractions 4.* (or 4.2.0) and
bring AppMetricsGraphite up to App.Metrics 4.2.0, then confirm the extension
code still matches the newer Abstractions and Concurrency APIs. Use the project
names Cassandra.IntegrationTests, Cassandra.AppMetrics, and AppMetricsGraphite
to locate the mismatched references.
- Line 36: The strict equality check in MetricsTests is comparing App.Metrics
gauge values that may now differ in type or precision under App.Metrics 4.x.
Update the assertion around GetNodeGauge and _metricsRoot.Snapshot.GetGaugeValue
to first confirm both sides are the same numeric type, and if they are
floating-point values, switch to a tolerance-based comparison instead of exact
equality.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa4f77b1-7344-4d7f-9499-7501ebcf523b
📒 Files selected for processing (1)
src/Cassandra.IntegrationTests/Cassandra.IntegrationTests.csproj
e40113f to
4aed567
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Cassandra.IntegrationTests/Metrics/MetricsTests.cs`:
- Line 300: The counter assertion in MetricsTests is too weak because checking
GetNodeCounter(...).GetValue() >= 0 will always pass even if the down-node path
stops updating metrics. Strengthen the invariant in the relevant test block
around MetricsTests and GetNodeCounter by asserting the expected counter
behavior under the node-down scenario, such as requiring a meaningful non-zero
change or a specific propagated value, so the test still verifies counter
activity rather than only non-negativity.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba79bf69-343d-4083-b40e-a7853c338840
📒 Files selected for processing (5)
examples/Metrics/AppMetricsGraphite/AppMetricsGraphite.csprojsrc/Cassandra.IntegrationTests/Cassandra.IntegrationTests.csprojsrc/Cassandra.IntegrationTests/Core/SessionTests.cssrc/Cassandra.IntegrationTests/Metrics/MetricsTests.cssrc/Extensions/Cassandra.AppMetrics/Cassandra.AppMetrics.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Cassandra.IntegrationTests/Cassandra.IntegrationTests.csproj
| { | ||
| Assert.AreEqual(0, metrics.GetNodeCounter(h, c).GetValue()); | ||
| } | ||
| Assert.GreaterOrEqual(metrics.GetNodeCounter(h, c).GetValue(), 0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This counter check is now effectively non-assertive.
>= 0 is a tautology for counters, so this block will still pass if the node-down path stops recording counter activity entirely. Please keep a stricter invariant here so the test still validates counter propagation under a down-node scenario.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Cassandra.IntegrationTests/Metrics/MetricsTests.cs` at line 300, The
counter assertion in MetricsTests is too weak because checking
GetNodeCounter(...).GetValue() >= 0 will always pass even if the down-node path
stops updating metrics. Strengthen the invariant in the relevant test block
around MetricsTests and GetNodeCounter by asserting the expected counter
behavior under the node-down scenario, such as requiring a meaningful non-zero
change or a specific propagated value, so the test still verifies counter
activity rather than only non-negativity.
1 participant
Supersedes #240.
This recreates and broadens Renovate's App.Metrics v4 bump on top of current master, which includes the shard-aware pool-balancing fix from #242.
The shipped AppMetrics extension and the integration-test consumer are now aligned on App.Metrics 4.3.0:
Cassandra.IntegrationTestsusesApp.Metrics4.3.0Cassandra.AppMetricsusesApp.Metrics.AbstractionsandApp.Metrics.Concurrency4.3.0Using 4.3.0 avoids the vulnerable legacy
NETStandard.Librarydependency path exposed byApp.Metrics.Concurrency4.2.0.Cassandra.IntegrationTestsalso pins patchedSystem.Text.Json/System.Text.RegularExpressionsversions so the restored graph does not retain vulnerable legacy transitive versions.AppMetricsGraphiteremains on its existing App.Metrics 3.2.0 reporter package in this PR. It still builds against the updated extension project, and changing that example manifest causes Snyk to scan/fail the example dependency tree independently of the shipped extension update.The replacement CI runs exposed unrelated/flaky integration-test assertions:
This PR keeps the same expectations where they are deterministic, replaces fixed sleeps with retry assertions for connection-pool counts, warms the relevant pools before checking shard-aware target sizes, treats node-down counters as valid non-negative metric values, and uses a tolerance for the App.Metrics gauge comparison.
Local verification:
Result: all local builds completed successfully. The AppMetrics extension has no vulnerable packages according to NuGet vulnerability metadata. The integration-test project build completed with 64 existing/expected unsupported-TFM warnings and 0 errors.