Skip failing TraceAnnotationsTests in .NET 6.0 by bouwkast · Pull Request #7552 · DataDog/dd-trace-dotnet

bouwkast · 2025-09-23T20:08:46Z

Summary of changes

Skip failing TraceAnnotationsTests in .NET 6.0

Reason for change

Started failing after #7287

Implementation details

Just a dumb Skip.If to skip for the time if we are in .NET 6.0

Test coverage

~~I missed this one in #7551 😢~~

I did the wrong one in #7551 after looking at the logs again

Other details

dd-trace-dotnet-ci-bot · 2025-09-23T21:01:38Z

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-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 shown in red. The following thresholds were used for comparing the execution times:

Welch test with statistical test for significance of 5%
Only results indicating a difference greater than 5% and 5 ms are considered.

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).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7552) - mean (74ms)  : 72, 75
     .   : milestone, 74,
    master - mean (74ms)  : 72, 75
     .   : milestone, 74,

    section Baseline
    This PR (7552) - mean (69ms)  : 67, 72
     .   : milestone, 69,
    master - mean (70ms)  : 69, 72
     .   : milestone, 70,

    section CallTarget+Inlining+NGEN
    This PR (7552) - mean (1,054ms)  : 1011, 1097
     .   : milestone, 1054,
    master - mean (1,065ms)  : 1006, 1124
     .   : milestone, 1065,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7552) - mean (109ms)  : 106, 111
     .   : milestone, 109,
    master - mean (109ms)  : 107, 111
     .   : milestone, 109,

    section Baseline
    This PR (7552) - mean (108ms)  : 105, 111
     .   : milestone, 108,
    master - mean (108ms)  : 105, 111
     .   : milestone, 108,

    section CallTarget+Inlining+NGEN
    This PR (7552) - mean (750ms)  : 727, 774
     .   : milestone, 750,
    master - mean (756ms)  : 730, 783
     .   : milestone, 756,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7552) - mean (96ms)  : 95, 98
     .   : milestone, 96,
    master - mean (98ms)  : 95, 101
     .   : milestone, 98,

    section Baseline
    This PR (7552) - mean (95ms)  : 93, 98
     .   : milestone, 95,
    master - mean (97ms)  : 94, 100
     .   : milestone, 97,

    section CallTarget+Inlining+NGEN
    This PR (7552) - mean (712ms)  : 690, 733
     .   : milestone, 712,
    master - mean (720ms)  : 685, 756
     .   : milestone, 720,

gantt
    title Execution time (ms) FakeDbCommand (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7552) - mean (94ms)  : 93, 96
     .   : milestone, 94,
    master - mean (97ms)  : 94, 99
     .   : milestone, 97,

    section Baseline
    This PR (7552) - mean (94ms)  : 91, 96
     .   : milestone, 94,
    master - mean (98ms)  : 96, 100
     .   : milestone, 98,

    section CallTarget+Inlining+NGEN
    This PR (7552) - mean (666ms)  : 652, 679
     .   : milestone, 666,
    master - mean (674ms)  : 658, 691
     .   : milestone, 674,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7552) - mean (205ms)  : 201, 208
     .   : milestone, 205,
    master - mean (204ms)  : 200, 209
     .   : milestone, 204,

    section Baseline
    This PR (7552) - mean (200ms)  : 196, 205
     .   : milestone, 200,
    master - mean (201ms)  : 196, 206
     .   : milestone, 201,

    section CallTarget+Inlining+NGEN
    This PR (7552) - mean (1,193ms)  : 1140, 1246
     .   : milestone, 1193,
    master - mean (1,194ms)  : 1121, 1267
     .   : milestone, 1194,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7552) - mean (289ms)  : 283, 294
     .   : milestone, 289,
    master - mean (289ms)  : 283, 296
     .   : milestone, 289,

    section Baseline
    This PR (7552) - mean (287ms)  : 281, 293
     .   : milestone, 287,
    master - mean (289ms)  : 282, 296
     .   : milestone, 289,

    section CallTarget+Inlining+NGEN
    This PR (7552) - mean (971ms)  : 920, 1022
     .   : milestone, 971,
    master - mean (961ms)  : 924, 999
     .   : milestone, 961,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7552) - mean (284ms)  : 278, 291
     .   : milestone, 284,
    master - mean (281ms)  : 276, 285
     .   : milestone, 281,

    section Baseline
    This PR (7552) - mean (291ms)  : 266, 315
     .   : milestone, 291,
    master - mean (280ms)  : 274, 286
     .   : milestone, 280,

    section CallTarget+Inlining+NGEN
    This PR (7552) - mean (954ms)  : 914, 994
     .   : milestone, 954,
    master - mean (948ms)  : 910, 987
     .   : milestone, 948,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7552) - mean (281ms)  : 275, 287
     .   : milestone, 281,
    master - mean (286ms)  : 275, 297
     .   : milestone, 286,

    section Baseline
    This PR (7552) - mean (281ms)  : 275, 286
     .   : milestone, 281,
    master - mean (283ms)  : 275, 291
     .   : milestone, 283,

    section CallTarget+Inlining+NGEN
    This PR (7552) - mean (876ms)  : 847, 905
     .   : milestone, 876,
    master - mean (882ms)  : 852, 912
     .   : milestone, 882,

## Summary of changes Fixes flakiness in the `TraceAnnotation` tests ## Reason for change #7287 incidentally introduced flakiness in the test when the profiler is not available (i.e. only on Windows). We temporarily disabled the test in #7551 and #7552. This reinstates the test, and removes the flake. ## Implementation details The important thing is that we do a "real" instrumentation in the app, to insure that we send instrumentation telemetry. Without this, there's a race condition between us instrumenting our "own" `HttpClient` usages and the app ending. In more detail, we see this flake because: - The integration telemetry is only sent when something is instrumented (or subsequently errors) - The delay in profiling introduced in #7287 causes a delay in sending the calltarget definitions - We will address that delay in a separate PR - The telemetry thread runs in the background, and is already started before we P/Invoke into the profiler - The delay causes us to not instrument the HttpClient calls that the background telemetry thread makes initially - if we skip the P/Invoke, then they are instrumented at this point - The app shuts down, which causes another telemetry flush - At this point the `HttpClient` calls are instrumented, and the instrumentation details are collected, but now we're shutting down, so this data is never sent. By forcing an instrumentation in the app, we bypass the race condition entirely. ## Test coverage Excluded the span from the tests so it's effectively the same. We don't care about the contents of that span (or whether the request passes or fails) - we just want to make sure we have instrumentation. ## Other details

… env var (#7555) ## Summary of changes Update profiler availability helper algorithm on Windows ## Reason for change - If the Continuous Profiler isn't available, the native loader does not rewrite P/Invokes and does not set `DD_INTERNAL_PROFILING_NATIVE_ENGINE_PATH` - If it is available, the native loader sets the `DD_INTERNAL_PROFILING_NATIVE_ENGINE_PATH` value - Currently, the `ProfilerAvailabilityHelper` is treating the presence of the variable as indication the profiler is available, but it's not treating the _absence_ as an indication the profiler is _not_ available. - This PR changes that to treat the `DD_INTERNAL_PROFILING_NATIVE_ENGINE_PATH` variable as the deciding factor on Windows (Note that his variable is _not_ available today on non-Windows as it doesn't propagate from the native side. We hope to fix that in the future, but in the mean time we must rely on heuristics). ## Implementation details Update the logic of the helper on Windows to only rely on the `DD_INTERNAL_PROFILING_NATIVE_ENGINE_PATH` variable. Note that I also removed the AAS Extension check, as that only runs on Windows anyway, so would never return `true` (as it's caught in the `IsWindows()` branch). ## Test coverage Updated the unit tests with new behaviour. Basically we need to skip more tests on Windows, as it's only governed by that flag ## Other details Related to the following: - #7554 - #7287 - #7552

Skip IntegrationDisabled as well

0549c06

bouwkast requested review from a team as code owners September 23, 2025 20:08

link04 approved these changes Sep 23, 2025

View reviewed changes

github-actions Bot added the area:tests unit tests, integration tests label Sep 23, 2025

SubmitTraces passed, so unskip it

1c51f93

bouwkast enabled auto-merge (squash) September 23, 2025 20:14

lucaspimentel approved these changes Sep 23, 2025

View reviewed changes

bouwkast merged commit 43e26c8 into master Sep 23, 2025
151 of 152 checks passed

bouwkast deleted the steven/skip-annotation-test2 branch September 23, 2025 21:29

github-actions Bot added this to the vNext-v3 milestone Sep 23, 2025

This was referenced Sep 24, 2025

Fix flakiness in the TraceAnnotation tests #7554

Merged

Update profiler availability helper to assume Windows always sets the env var #7555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Skip failing TraceAnnotationsTests in .NET 6.0#7552

Skip failing TraceAnnotationsTests in .NET 6.0#7552
bouwkast merged 2 commits into
masterfrom
steven/skip-annotation-test2

bouwkast commented Sep 23, 2025 •

edited

Loading

Uh oh!

dd-trace-dotnet-ci-bot Bot commented Sep 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

bouwkast commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

dd-trace-dotnet-ci-bot Bot commented Sep 23, 2025

Execution-Time Benchmarks Report ⏱️

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bouwkast commented Sep 23, 2025 •

edited

Loading