Remove DiagnosticSource.Write trim warnings in SqlDiagnosticListener - #4688
charlesroddie wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The trimming guarantees need a publish-trimmed test covering the preserved nested members.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Centralizes diagnostic event writes to suppress redundant trim warnings while preserving reflected payload members.
Changes:
- Routes all 15 diagnostic writes through
WriteEvent<T>. - Adds trimming annotations and nested-member dependencies.
File summaries
| File | Description |
|---|---|
SqlDiagnosticListener.cs |
Adds the annotated diagnostic-write helper. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
0760c29 to
f621741
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
A trimmed runtime test is needed to validate that the reflection-dependent members survive linking.
Review details
Suppressed comments (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlDiagnosticListener.cs:490
- The PR's core runtime guarantee is not exercised: an AOT-compatible build only shows that IL2026 is suppressed, while the existing diagnostic tests run untrimmed and cannot prove that these two reflected members survive linking. Please add a trimmed-publish smoke test that subscribes to an error event, reflects
SqlException.Number, and resolves/invokesSqlConnection.RetrieveStatistics; otherwise a misplaced or ineffective dependency annotation would pass all current validation but break OpenTelemetry in trimmed applications.
[DynamicDependency(nameof(SqlException.Number), typeof(SqlException))]
[DynamicDependency(nameof(SqlConnection.RetrieveStatistics), typeof(SqlConnection))]
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Route the 15 diagnostic events through one WriteEvent helper that carries the PublicProperties annotation on T required by Write<T> and suppresses IL2026 once. SqlClient does no reflection when writing events; subscribers that reflect over payloads are responsible for their own reflection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f621741 to
9be4c69
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused refactoring preserves existing event behavior while correctly centralizing trim annotations and suppression.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Description
Removes the 15 IL2026 warnings in
SqlDiagnosticListener, one perDiagnosticSource.Write<T>call.The
[RequiresUnreferencedCode]attribute onWrite<T>is in fact untrue!Write<T>does no reflection intrinsically: it passes the payload to subscribers asobject. The reason it's there is that subscribers could reflect over the payload. However that is their responsibility, and they will receive their own trim warnings when they do.So this attribute was always misguided, and so suppressing it is common, e.g. in
DiagnosticsHandlerin dotnet/runtime andHostingApplicationDiagnosticsin ASP.NET Core. Following those examples, all events now go through one privateWriteEvent<T>helper with anUnconditionalSuppressMessageonIL2026.This helper's
Thas the samePublicPropertiesannotation asWrite<T>. This is pragmatic. This keeps existing consumers working, so this PR doesn't change behaviour under trimming and only corrects trim warnings. Some of these consumers, including the current OpenTelemetry SqlClient implementation, not only require these properties but suppress their own warnings, so removing this annotation would not only stop code from working under NativeAOT, but do so silently.The claims above were checked in a NativeAOT test app: see details below.
Trim/AOT warnings with
-p:IsAotCompatible=true(unique by location and code):No public API changes. Independent of #4683 and #4684.
Issues
Part of #1947.
Testing
-p:IsAotCompatible=true; no warnings remain inSqlDiagnosticListener.cs.DiagnosticTest(21 tests covering connection open and command execution events, sync, async and error paths) passes on net8.0.NativeAOT verification of Description claims
A console app (net10.0,
PublishAot, win-x64) subscribed toSqlClientDiagnosticListenerwith a reflecting observer (GetType().GetProperty, once unsuppressed and once withUnconditionalSuppressMessage) and withOpenTelemetry.Instrumentation.SqlClient1.18.0 plus an in-memory exporter, then ranSELECT 1andSELECT 1/0against LocalDB. It was published againstmain, this PR, and this PR with thePublicPropertiesannotation removed.mainSqlDiagnosticListenertrim warningsCommand/Connectionreadable by reflectionmainshows 9 warnings rather than 15 because the app doesn't use transactions.db.response.status_code(present under JIT), becauseSqlException.Numberisn't reflectable. This PR doesn't change that.🤖 Generated with Claude Code