Reduce warnings in test-applications samples#8519
Merged
Merged
Conversation
NachoEchevarria
changed the title
Restores the System.Net.*/System.IO.*/System.Threading.Channels
PackageReferences I had deleted for NU1510 in Samples.Elasticsearch,
Samples.Elasticsearch.V7, Samples.RabbitMQ, Samples.DataStreams.RabbitMQ,
and LogsInjection.Serilog. Their original comments ("avoid package
downgrade errors") were accurate: they resolve transitive NU1605
conflicts introduced by NEST 6.1.0 / Serilog.Sinks.File 2.2.0 /
RabbitMQ.Client < 5 via NETStandard.Library 1.6.0. dotnet restore is
lenient about these but nuget.exe restore (CI) fails hard with NU1605.
Adds <NoWarn>NU1510</NoWarn> per csproj with a comment explaining the
refs are retained for conflict resolution despite being flagged as
framework-provided.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8519) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8519) - mean (72ms) : 70, 75
master - mean (73ms) : 70, 76
section Bailout
This PR (8519) - mean (80ms) : 75, 85
master - mean (80ms) : 76, 84
section CallTarget+Inlining+NGEN
This PR (8519) - mean (1,128ms) : 1067, 1189
master - mean (1,121ms) : 1073, 1169
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8519) - mean (117ms) : 111, 124
master - mean (117ms) : 111, 123
section Bailout
This PR (8519) - mean (116ms) : 111, 121
master - mean (115ms) : 111, 119
section CallTarget+Inlining+NGEN
This PR (8519) - mean (807ms) : 785, 829
master - mean (807ms) : 777, 837
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8519) - mean (100ms) : 96, 104
master - mean (103ms) : 97, 109
section Bailout
This PR (8519) - mean (105ms) : 98, 111
master - mean (105ms) : 100, 109
section CallTarget+Inlining+NGEN
This PR (8519) - mean (943ms) : 907, 979
master - mean (942ms) : 902, 982
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8519) - mean (100ms) : 96, 104
master - mean (99ms) : 96, 102
section Bailout
This PR (8519) - mean (101ms) : 98, 104
master - mean (103ms) : 98, 108
section CallTarget+Inlining+NGEN
This PR (8519) - mean (836ms) : 773, 898
master - mean (831ms) : 784, 879
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8519) - mean (205ms) : 194, 216
master - mean (199ms) : 192, 205
section Bailout
This PR (8519) - mean (203ms) : 198, 207
master - mean (203ms) : 197, 210
section CallTarget+Inlining+NGEN
This PR (8519) - mean (1,225ms) : 1184, 1266
master - mean (1,226ms) : 1181, 1271
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8519) - mean (285ms) : 273, 296
master - mean (283ms) : 274, 293
section Bailout
This PR (8519) - mean (286ms) : 274, 298
master - mean (284ms) : 277, 292
section CallTarget+Inlining+NGEN
This PR (8519) - mean (972ms) : 946, 997
master - mean (982ms) : 949, 1014
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8519) - mean (278ms) : 268, 288
master - mean (282ms) : 269, 294
section Bailout
This PR (8519) - mean (280ms) : 271, 289
master - mean (285ms) : 273, 296
section CallTarget+Inlining+NGEN
This PR (8519) - mean (1,153ms) : 1119, 1187
master - mean (1,157ms) : 1122, 1192
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8519) - mean (278ms) : 270, 286
master - mean (282ms) : 272, 292
section Bailout
This PR (8519) - mean (278ms) : 269, 287
master - mean (282ms) : 270, 294
section CallTarget+Inlining+NGEN
This PR (8519) - mean (1,038ms) : 993, 1084
master - mean (1,041ms) : 993, 1090
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-04-30 09:54:45 Comparing candidate commit 019f0c7 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics, 60 known flaky benchmarks, 27 flaky benchmarks without significant changes.
|
NachoEchevarria
added
the
AI Generated
NachoEchevarria
marked this pull request as ready for review
April 27, 2026 14:40
Merged
andrewlock
approved these changes
Apr 29, 2026
Member
There was a problem hiding this comment.
Thanks, this is great!
FYI, I started an "all frameworks run" here, just to make sure we don't have any issues in earlier TFMs
|
|
||
| var tracer = GetTracerInstance.Invoke(null, Array.Empty<object>()); | ||
| return (IDisposable)StartActiveMethod.Invoke(tracer, new object[] { operationName }); | ||
| return (IDisposable?)StartActiveMethod.Invoke(tracer, new object[] { operationName }) ?? new NoOpDisposable(); |
Member
There was a problem hiding this comment.
Should this not return null, isn't this a change in behaviour? 🤔
Collaborator
Author
There was a problem hiding this comment.
Yes, technically a behavior change, but one that I would keep. The function declares IDisposable (non-null), and the original code returned null only when Invoke returned null while returning NoOpDisposable for other failures. The new code makes both paths consistent with the declared return type. Also, all callers use using (...) which handles both equivalently.
…es.MySql.csproj Co-authored-by: Andrew Lock <[email protected]>
NachoEchevarria
added a commit
that referenced
this pull request
Apr 29, 2026
## Summary of changes Eliminates the 32 native C++ build warnings (15 unique source positions) emitted by the Windows native build. ## Reason for change Follow-up to #8519 (sample warnings). Native was the next cluster where every warning is a real fix rather than structural noise. ## Implementation details - **C4018** (signed/unsigned `<`): `auto x = 0` → `size_t x = 0` in 5 loops in `rejit_handler.cpp` and `int i = 2` → `size_t i = 2` in shared `cor_profiler.cpp`. Loop vars only used as indices. - **C4267 / C4244** (`size_t` / `ptrdiff_t` narrowing into `ULONG` / `DWORD` / `int` API params): explicit `static_cast` at the call sites in `calltarget_tokens.cpp`, `fault_tolerant_rewriter.cpp`, `process_helper.cpp`, `tracer_tokens.cpp`, and the two generated `.g.cpp` files. All inputs bounded well below the cast range. The corresponding generator templates in `tracer/build/_build/CodeGenerators/` are updated alongside the generated outputs. - **`SignatureBuilder::EnsureBufferSpace`** param `int` → `size_t` to match the rest of the class. Also clears a latent signed/unsigned mix in `_offset + size >= _length`. - **`fault_tolerant_rewriter.cpp:86,97`**: `std::string(wstr.begin(), wstr.end())` → `shared::ToString(wstr)` (the C4244 trigger inside `<xstring>`). Identical behavior for ASCII generic-type names; UTF-8-correct for non-ASCII where the old form silently truncated. ## Test coverage ## Other details
e-n-0
approved these changes
Apr 30, 2026
NachoEchevarria
added a commit
that referenced
this pull request
May 7, 2026
## Summary of changes Part 3 of the warning-reduction series. ~25 warning sources eliminated across sample/regression projects and the runner tool. Previous PRs: #8534 #8519 ## Reason for change CI emits ~17k warnings; chipping away at the batch surfaced after parts 1-2 merged. ## Implementation details - **Deleted redundant `PackageReference`s** (NU1510 — flagged framework-provided on the target TFM and not needed): `Microsoft.AspNetCore.Mvc.RazorPages`, `Microsoft.Extensions.Logging.Debug`, `System.Runtime.InteropServices.RuntimeInformation`, `System.Data.Common`, `System.Net.Http 4.3.4`, `System.IO.FileSystem`, `System.Net.NameResolution`. - **Targeted `<NoWarn>` (with comment explaining why the reference is load-bearing)**: - NU1510 for `System.Memory` / `System.Net.Http` on net4x targets — Probes, CallTargetNativeTest, VersionConflict.{1x,2x}, DuplicateTypeProxy. - MSTEST0001/0044/0057 + NU1701 — MSTestTests, MSTestTests2, MSTestTestsRetries. - NETSDK1201 — ManualInstrumentation; ASP0014 — Ocelot.DistributedTracing. - IL2104 — `Datadog.Trace.Tools.Runner` (third-party assemblies already rooted via `<TrimmerRootAssembly>`). - **Nullability fixes**: `SampleHelpers.TrySetServiceName` parameter widened to `string?` with an early-return guard so we don't reflection-invoke the setter with null; redundant `evaluation?.Error` removed in `Samples.FeatureFlags/Evaluator.cs` (was de-narrowing the flow state and tripping CS8602 on net48); `Samples.GrpcDotNet/Worker.cs` `[CallerMemberName]` default fixed. ## Test coverage No behavioral changes. CI verifies the warning count drops without breaking compilation. ## Other details --------- Co-authored-by: Andrew Lock <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Reduces build warnings in
tracer/test/test-applications/**. On the "Build sample projects" CI log I sampled, the count drops from ~1,650 to ~18.Reason for change
~17k warnings across a full CI pipeline have largely lost their signal. This is a first pass, scoped to the samples because it's the lowest-risk surface to clean up.
Implementation details
Samples.Shared(compiled into most consumers via.shproj):SampleHelpers.csandWebServer.csget#nullable enablewith proper annotations on reflection-lookup fields and method parameters. Several latent NREs the analyzer was flagging are now explicitly guarded. Two API-visible refinements:GetActiveSpanContext()return type changedobject→object?. Failure paths now returnnullinstead of aNoOpDisposablesentinel — callers reflect over the result viaMethodInfo.Invoke(target, …), which wouldTargetExceptionon a wrong-type sentinel.GetTraceId/GetSpanId/GetOrMakeSamplingDecisionupdated to short-circuit on null context.InjectScope(…, object scope)→(…, object? scope)with ascope is nullguard, since its only documented external caller (Samples.Wcf/CalculatorService.cs) feedsGetActiveSpanContext()straight in.TrackUserLogin{Success,Failure}EventMethodlookups moved from field initializers into the static constructor, removing aTracerType!shadow cast that worked only because of incidental co-nullity withEventTrackingSdk.StackExchange.Redis.AssemblyConflict.LegacyProjectbumped<LangVersion>from7.3tolatest(moved to the top-levelPropertyGroup). The project regression-tests assembly-conflict resolution, not C# language version.<LangVersion>latest</LangVersion>added (Samples.AspNet.VersionConflict,Samples.AspNet472.LoaderOptimizationRegKey,Samples.AspNetAsyncHandler,Samples.AspNetMvc4,Samples.AspNetMvc5,Samples.Owin.Iis.WebApi2,Samples.WebForms,Samples.Security.WebApi,Samples.Security.WebForms). The default for old-format csprojs on net48 is C# 7.3, which rejects#nullable/Type?/!.Samples.AspNet.VersionConflictandSamples.AspNetMvc5additionally bump their pinnedMicrosoft.Net.Compilersfrom 2.10.0 → 4.0.1 (last stable before the.Toolsetrename; supports up to C# 10). Without this, even with<LangVersion>latest</LangVersion>set, the pinned 2.10.0 Roslyn doesn't recognize C# 8 syntax.<PackageReference>pins for legacy framework-assembly shims (System.Net.Requests,Microsoft.AspNetCore 2.2,System.Diagnostics.DiagnosticSource,System.Runtime.CompilerServices.Unsafe,System.ValueTuple, etc.) that are now part of the shared framework on every TFM these projects target.Samples.MySql'sSystem.Text.Encoding.CodePagesis kept behindCondition="$(TargetFramework.StartsWith('net4')) OR $(TargetFramework.StartsWith('netcoreapp'))"—CodePagesEncodingProvideronly entered the shared framework in .NET 5.Samples.Elasticsearch,Samples.Elasticsearch.V7,Samples.RabbitMQ,Samples.DataStreams.RabbitMQ,LogsInjection.Serilog. Their original comments explicitly said "avoid package downgrade errors" — the refs prevent transitiveNU1605conflicts fromNEST 6.1.0/Serilog.Sinks.File 2.2.0/ oldRabbitMQ.ClientviaNETStandard.Library 1.6.0. First attempt deleted them and CI failed onnuget.exe restore(which is stricter thandotnet restore); the refs are retained with<NoWarn>NU1510</NoWarn>and a comment on each csproj.<NoWarn>on samples that intentionally exercise deprecated APIs (each csproj carries a short comment explaining what's suppressed and why):CS0618onSqlServer,StackExchange.Redis,MongoDB,Couchbase3,AWS.SQS,Hangfire;MSTEST0044/0057onMSTestTests;CS8981onGoogleProtobuf(protoc-generated lowercase aliases);NETSDK1206onSqlServer(transitivewin7-*RIDs in the legacySystem.Data.SqlClientnative dep).Samples.OpenFeature/Evaluator.cs:client = null!field init, removed a redundant?.that was confusing flow analysis,!after null-asserting helper calls.Test coverage
Each modified sample under the modern SDK style was built locally with
dotnet build -c Releaseacross its full TFM matrix — all produce 0 warnings / 0 errors.LegacyProjectre-verified after theLangVersionbump. The old-style ASP.NET csprojs (which needmsbuild+nuget.exe restore) couldn't be locally verified end-to-end and rely on CI; the changes there are mechanical (LangVersion / package-version edits). No new tests; this is a build-warning cleanup.Other details
Samples.AWS.DynamoDBv2(18 occ). The stale minimum versions live in generatedPackageVersionsLatest*.g.props; fixing them requires running theGeneratePackageVersionsNuke target, which does a full tracer rebuild. Disproportionate for 18 cosmetic warnings — will self-heal on the next scheduled matrix regen.<LangVersion>latest</LangVersion>was added, IL output is effectively unchanged (same machine Roslyn, just unlocking newer syntax that the project's own code doesn't use). For the 2 withMicrosoft.Net.Compilers 2.10 → 4.0.1, Roslyn version differs so IL details (lambda lowering, async state machines) can differ but the C# semantics are equivalent — worth noting thatSamples.AspNet.VersionConflictexists specifically to test assembly-conflict resolution, which depends on assembly references at runtime, not on compiler IL choices.