Skip to content

[CallTarget] Flag to skip method execution #6843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Apr 15, 2025

Conversation

tonyredondo
Copy link
Member

@tonyredondo tonyredondo commented Apr 11, 2025

Summary of changes

This pull request introduces a new feature to the CallTargetState struct, allowing for the skipping of method body execution. The changes span multiple files, primarily within the tracer directory, and include updates to constructors, method definitions, and test cases.

Reason for change

This feature was in the backlog

Implementation details

Key changes include:

Enhancements to CallTargetState:

  • Added a new private readonly field _skipMethodBody to the CallTargetState struct and updated its constructors to include a skipMethodBody parameter. (tracer/src/Datadog.Trace/ClrProfiler/CallTarget/CallTargetState.cs) [1] [2] [3] [4]
  • Implemented a new property SkipMethodBody to expose the _skipMethodBody field. (tracer/src/Datadog.Trace/ClrProfiler/CallTarget/CallTargetState.cs)
  • Updated the ToString method to include the _skipMethodBody field. (tracer/src/Datadog.Trace/ClrProfiler/CallTarget/CallTargetState.cs)

Updates to Native Code:

  • Introduced a new member reference for get_SkipMethodBody in calltarget_tokens.cpp and calltarget_tokens.h. (tracer/src/Datadog.Tracer.Native/calltarget_tokens.cpp, tracer/src/Datadog.Tracer.Native/calltarget_tokens.h) [1] [2] [3] [4] [5]
  • Modified the TracerMethodRewriter to check the SkipMethodBody property and conditionally skip the method body execution. (tracer/src/Datadog.Tracer.Native/tracer_method_rewriter.cpp) [1] [2] [3]

Test Case Adjustments:

  • Added a constant SKIPMETHODBODY and logic to handle the skipping of method bodies in Noop1ArgumentsIntegration. (tracer/test/test-applications/instrumentation/CallTargetNativeTest/NoOp/Noop1ArgumentsIntegration.cs) [1] [2] [3]
  • Updated With1Arguments to test the new SkipMethodBody functionality. (tracer/test/test-applications/instrumentation/CallTargetNativeTest/With1Arguments.cs) [1] [2]

Test coverage

The CallTargetNative test app was update with two cases testing the skip method flag.

Other details

@tonyredondo tonyredondo marked this pull request as ready for review April 11, 2025 13:57
@tonyredondo tonyredondo requested a review from a team as a code owner April 11, 2025 13:57
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Apr 12, 2025

Datadog Report

Branch report: tony/calltarget-skip-method-body
Commit report: 5f72db5
Test service: dd-trace-dotnet

✅ 0 Failed, 6 Passed, 0 Skipped, 0s Total Time

@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot Apr 14, 2025
@DataDog DataDog deleted a comment from andrewlock Apr 14, 2025
@DataDog DataDog deleted a comment from andrewlock Apr 14, 2025
@andrewlock
Copy link
Member

andrewlock commented Apr 14, 2025

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.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6843) - mean (70ms)  : 67, 73
     .   : milestone, 70,
    master - mean (70ms)  : 67, 72
     .   : milestone, 70,

    section CallTarget+Inlining+NGEN
    This PR (6843) - mean (1,011ms)  : 989, 1033
     .   : milestone, 1011,
    master - mean (1,009ms)  : 988, 1031
     .   : milestone, 1009,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6843) - mean (104ms)  : 102, 106
     .   : milestone, 104,
    master - mean (104ms)  : 102, 106
     .   : milestone, 104,

    section CallTarget+Inlining+NGEN
    This PR (6843) - mean (701ms)  : 681, 720
     .   : milestone, 701,
    master - mean (697ms)  : 677, 716
     .   : milestone, 697,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6843) - mean (90ms)  : 88, 92
     .   : milestone, 90,
    master - mean (90ms)  : 88, 92
     .   : milestone, 90,

    section CallTarget+Inlining+NGEN
    This PR (6843) - mean (659ms)  : 639, 679
     .   : milestone, 659,
    master - mean (658ms)  : 634, 682
     .   : milestone, 658,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6843) - mean (190ms)  : 186, 193
     .   : milestone, 190,
    master - mean (189ms)  : 186, 193
     .   : milestone, 189,

    section CallTarget+Inlining+NGEN
    This PR (6843) - mean (1,115ms)  : 1086, 1143
     .   : milestone, 1115,
    master - mean (1,105ms)  : 1084, 1126
     .   : milestone, 1105,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6843) - mean (268ms)  : 264, 271
     .   : milestone, 268,
    master - mean (268ms)  : 264, 272
     .   : milestone, 268,

    section CallTarget+Inlining+NGEN
    This PR (6843) - mean (880ms)  : 849, 911
     .   : milestone, 880,
    master - mean (876ms)  : 852, 901
     .   : milestone, 876,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6843) - mean (260ms)  : 257, 264
     .   : milestone, 260,
    master - mean (261ms)  : 257, 265
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (6843) - mean (870ms)  : 839, 900
     .   : milestone, 870,
    master - mean (863ms)  : 836, 891
     .   : milestone, 863,

Loading

@andrewlock
Copy link
Member

andrewlock commented Apr 14, 2025

Benchmarks Report for tracer 🐌

Benchmarks for #6843 compared to master:

  • 6 benchmarks are faster, with geometric mean 1.150
  • 1 benchmarks have fewer allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.ActivityBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #6843

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.ActivityBenchmark.StartStopWithChild‑net6.0 5.6 KB 5.57 KB -34 B -0.61%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 7.93μs 133ns 1.32μs 0 0 0 5.6 KB
master StartStopWithChild netcoreapp3.1 10.4μs 120ns 1.2μs 0 0 0 5.77 KB
master StartStopWithChild net472 15.2μs 85.6ns 617ns 0.989 0.282 0.0706 6.13 KB
#6843 StartStopWithChild net6.0 7.7μs 92.8ns 923ns 0 0 0 5.57 KB
#6843 StartStopWithChild netcoreapp3.1 10.4μs 174ns 1.74μs 0 0 0 5.76 KB
#6843 StartStopWithChild net472 14.7μs 37.9ns 137ns 1.03 0.316 0.0791 6.16 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 512μs 1.92μs 7.43μs 0 0 0 2.71 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 685μs 1.01μs 3.79μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 865μs 343ns 1.24μs 0 0 0 3.31 KB
#6843 WriteAndFlushEnrichedTraces net6.0 488μs 474ns 1.71μs 0 0 0 2.7 KB
#6843 WriteAndFlushEnrichedTraces netcoreapp3.1 643μs 1.68μs 6.27μs 0 0 0 2.7 KB
#6843 WriteAndFlushEnrichedTraces net472 859μs 886ns 3.43μs 0 0 0 3.31 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net6.0 130μs 698ns 4.01μs 0 0 0 14.48 KB
master SendRequest netcoreapp3.1 147μs 726ns 3.25μs 0 0 0 17.28 KB
master SendRequest net472 0.000445ns 0.000305ns 0.00114ns 0 0 0 0 b
#6843 SendRequest net6.0 129μs 724ns 4.86μs 0 0 0 14.48 KB
#6843 SendRequest netcoreapp3.1 147μs 333ns 1.2μs 0 0 0 17.28 KB
#6843 SendRequest net472 0.00428ns 0.00128ns 0.00496ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 551μs 1.01μs 3.65μs 0 0 0 41.43 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 709μs 6.26μs 61.6μs 0 0 0 41.8 KB
master WriteAndFlushEnrichedTraces net472 857μs 5.71μs 56.8μs 7.81 0 0 53.28 KB
#6843 WriteAndFlushEnrichedTraces net6.0 565μs 4.26μs 40.9μs 0 0 0 41.57 KB
#6843 WriteAndFlushEnrichedTraces netcoreapp3.1 650μs 3.27μs 15.3μs 0 0 0 41.7 KB
#6843 WriteAndFlushEnrichedTraces net472 912μs 5.39μs 53.3μs 8.33 4.17 0 53.39 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 1.35μs 3.5ns 13.5ns 0.0133 0 0 1.03 KB
master ExecuteNonQuery netcoreapp3.1 1.76μs 2.32ns 8.69ns 0.00872 0 0 1.02 KB
master ExecuteNonQuery net472 2.09μs 4.08ns 15.8ns 0.155 0.0103 0 995 B
#6843 ExecuteNonQuery net6.0 1.33μs 2.44ns 9.11ns 0.0134 0 0 1.03 KB
#6843 ExecuteNonQuery netcoreapp3.1 1.81μs 5.34ns 19.2ns 0.00899 0 0 1.02 KB
#6843 ExecuteNonQuery net472 2.05μs 6.18ns 23.9ns 0.152 0.0102 0 995 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.23μs 1.22ns 4.74ns 0.0124 0 0 984 B
master CallElasticsearch netcoreapp3.1 1.65μs 1.88ns 6.79ns 0.00809 0 0 984 B
master CallElasticsearch net472 2.57μs 1.99ns 7.72ns 0.154 0 0 1 KB
master CallElasticsearchAsync net6.0 1.3μs 0.927ns 3.59ns 0.0129 0 0 960 B
master CallElasticsearchAsync netcoreapp3.1 1.68μs 1.91ns 7.13ns 0.00842 0 0 1.03 KB
master CallElasticsearchAsync net472 2.68μs 1.44ns 5.56ns 0.161 0 0 1.06 KB
#6843 CallElasticsearch net6.0 1.25μs 1.24ns 4.82ns 0.0127 0 0 984 B
#6843 CallElasticsearch netcoreapp3.1 1.57μs 2.15ns 8.05ns 0.00781 0 0 984 B
#6843 CallElasticsearch net472 2.53μs 1.65ns 6.37ns 0.151 0 0 1 KB
#6843 CallElasticsearchAsync net6.0 1.22μs 0.643ns 2.41ns 0.0123 0 0 960 B
#6843 CallElasticsearchAsync netcoreapp3.1 1.66μs 1.02ns 3.69ns 0.00851 0 0 1.03 KB
#6843 CallElasticsearchAsync net472 2.72μs 1.25ns 4.67ns 0.164 0 0 1.06 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net6.0 1.37μs 0.829ns 3.21ns 0.0138 0 0 960 B
master ExecuteAsync netcoreapp3.1 1.67μs 4.72ns 17.7ns 0.00852 0 0 960 B
master ExecuteAsync net472 1.74μs 0.624ns 2.33ns 0.139 0 0 923 B
#6843 ExecuteAsync net6.0 1.44μs 1.01ns 3.78ns 0.00721 0 0 960 B
#6843 ExecuteAsync netcoreapp3.1 1.7μs 1.02ns 3.95ns 0.00818 0 0 960 B
#6843 ExecuteAsync net472 1.84μs 0.633ns 2.45ns 0.138 0 0 923 B
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net6.0 4.34μs 3.29ns 12.7ns 0.0217 0 0 2.32 KB
master SendAsync netcoreapp3.1 5.38μs 4.59ns 17.2ns 0.027 0 0 2.86 KB
master SendAsync net472 7.53μs 3.83ns 14.8ns 0.49 0 0 3.13 KB
#6843 SendAsync net6.0 4.35μs 2.67ns 10.4ns 0.0217 0 0 2.32 KB
#6843 SendAsync netcoreapp3.1 5.33μs 3.85ns 13.9ns 0.0268 0 0 2.86 KB
#6843 SendAsync net472 7.55μs 3.55ns 12.3ns 0.487 0 0 3.13 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 1.49μs 1.31ns 5.08ns 0.0223 0 0 1.71 KB
master EnrichedLog netcoreapp3.1 2.24μs 2.3ns 7.98ns 0.0227 0 0 1.71 KB
master EnrichedLog net472 2.59μs 4.33ns 16.2ns 0.26 0 0 1.64 KB
#6843 EnrichedLog net6.0 1.48μs 1.53ns 5.92ns 0.0223 0 0 1.71 KB
#6843 EnrichedLog netcoreapp3.1 2.31μs 2.67ns 9.99ns 0.0115 0 0 1.71 KB
#6843 EnrichedLog net472 2.71μs 1.21ns 4.54ns 0.257 0 0 1.64 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 113μs 283ns 1.1μs 0 0 0 4.32 KB
master EnrichedLog netcoreapp3.1 116μs 214ns 831ns 0 0 0 4.32 KB
master EnrichedLog net472 150μs 173ns 646ns 0 0 0 4.51 KB
#6843 EnrichedLog net6.0 114μs 352ns 1.36μs 0 0 0 4.32 KB
#6843 EnrichedLog netcoreapp3.1 115μs 176ns 660ns 0 0 0 4.32 KB
#6843 EnrichedLog net472 153μs 357ns 1.38μs 0 0 0 4.51 KB
Benchmarks.Trace.NLogBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6843

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.NLogBenchmark.EnrichedLog‑net6.0 1.122 3,245.75 2,892.59

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 3.25μs 2.28ns 8.54ns 0.0331 0 0 2.26 KB
master EnrichedLog netcoreapp3.1 4.26μs 2.3ns 8.3ns 0.0214 0 0 2.26 KB
master EnrichedLog net472 4.91μs 1.49ns 5.76ns 0.32 0 0 2.09 KB
#6843 EnrichedLog net6.0 2.9μs 5.94ns 23ns 0.0291 0 0 2.26 KB
#6843 EnrichedLog netcoreapp3.1 4.12μs 4.94ns 18.5ns 0.0208 0 0 2.26 KB
#6843 EnrichedLog net472 4.92μs 2.19ns 8.19ns 0.32 0 0 2.09 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net6.0 1.46μs 1.26ns 4.72ns 0.0146 0 0 1.15 KB
master SendReceive netcoreapp3.1 1.69μs 3.65ns 14.1ns 0.00839 0 0 1.15 KB
master SendReceive net472 2.01μs 1.8ns 6.75ns 0.18 0 0 1.16 KB
#6843 SendReceive net6.0 1.45μs 1.17ns 4.52ns 0.0146 0 0 1.15 KB
#6843 SendReceive netcoreapp3.1 1.86μs 8.74ns 32.7ns 0.00915 0 0 1.15 KB
#6843 SendReceive net472 2.03μs 1.18ns 4.24ns 0.183 0 0 1.16 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 2.87μs 3.1ns 11.6ns 0.0141 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 4.02μs 3.37ns 12.6ns 0.0201 0 0 1.69 KB
master EnrichedLog net472 4.57μs 5ns 18ns 0.317 0 0 2.08 KB
#6843 EnrichedLog net6.0 2.9μs 1.46ns 5.48ns 0.0146 0 0 1.64 KB
#6843 EnrichedLog netcoreapp3.1 3.92μs 3.1ns 11.2ns 0.0195 0 0 1.69 KB
#6843 EnrichedLog net472 4.5μs 2.86ns 10.3ns 0.315 0 0 2.08 KB
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6843

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑netcoreapp3.1 1.211 819.80 677.16
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 1.178 467.77 397.24
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 1.143 556.05 486.41

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 468ns 0.38ns 1.47ns 0.00711 0 0 584 B
master StartFinishSpan netcoreapp3.1 563ns 0.209ns 0.783ns 0.00568 0 0 584 B
master StartFinishSpan net472 675ns 0.205ns 0.793ns 0.0903 0 0 586 B
master StartFinishScope net6.0 556ns 0.449ns 1.68ns 0.0083 0 0 704 B
master StartFinishScope netcoreapp3.1 820ns 0.49ns 1.77ns 0.00803 0 0 704 B
master StartFinishScope net472 866ns 0.322ns 1.25ns 0.104 0 0 666 B
#6843 StartFinishSpan net6.0 397ns 0.253ns 0.946ns 0.00792 0 0 584 B
#6843 StartFinishSpan netcoreapp3.1 572ns 0.773ns 2.89ns 0.00568 0 0 584 B
#6843 StartFinishSpan net472 729ns 0.292ns 1.09ns 0.0913 0 0 586 B
#6843 StartFinishScope net6.0 487ns 0.289ns 1.04ns 0.00975 0 0 704 B
#6843 StartFinishScope netcoreapp3.1 678ns 0.948ns 3.55ns 0.00678 0 0 704 B
#6843 StartFinishScope net472 855ns 0.643ns 2.49ns 0.102 0 0 666 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6843

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 1.135 690.10 607.81
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑netcoreapp3.1 1.116 979.82 877.65

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 689ns 0.829ns 3.21ns 0.00694 0 0 704 B
master RunOnMethodBegin netcoreapp3.1 983ns 1.75ns 6.79ns 0.00505 0 0 704 B
master RunOnMethodBegin net472 1.13μs 0.674ns 2.52ns 0.102 0 0 666 B
#6843 RunOnMethodBegin net6.0 608ns 0.509ns 1.97ns 0.00912 0 0 704 B
#6843 RunOnMethodBegin netcoreapp3.1 877ns 1.28ns 4.94ns 0.0088 0 0 704 B
#6843 RunOnMethodBegin net472 1.1μs 0.569ns 2.2ns 0.101 0 0 666 B

Copy link
Member

@andrewlock andrewlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks!

Comment on lines +131 to +133
/// <summary>
/// Gets the previous scope
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that really clears things up - let me guess, more copilot suggestions? 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, without copilot, sometimes I leave comments like these without realising how obvious they are 😄

@tonyredondo tonyredondo requested a review from a team as a code owner April 14, 2025 09:30
Copy link
Contributor

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@tonyredondo tonyredondo merged commit a16a781 into master Apr 15, 2025
130 of 132 checks passed
@tonyredondo tonyredondo deleted the tony/calltarget-skip-method-body branch April 15, 2025 23:21
@github-actions github-actions bot added this to the vNext-v3 milestone Apr 15, 2025
Comment on lines +139 to +140
// Enables or disables the skip method body feature
const shared::WSTRING internal_skip_method_body_enabled = WStr("DD_INTERNAL_SKIP_METHOD_BODY_ENABLED");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary 🤔 It looks like it was something you thought you would need when trying to figure out the issue, but it isn't actually necessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's more, it looks essentially dangerous, this doesn't seem like it's something that should be configurable 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a separate PR to remove this and tweak some things: #6861

@andrewlock andrewlock added type:enhancement Improvement to an existing feature area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) labels Apr 16, 2025
andrewlock added a commit that referenced this pull request Apr 16, 2025
## Summary of changes

- ~Remove the "kill switch" for the "skip method execution" filter~
- Refactor the `CallTargetState` constructors

## Reason for change

#6843 introduced a new
flag to allow bypassing the target method. This required introducing a
new parameter, `skipMethodBody`, to the `CallTargetState` constructors.
However, this was added as an optional parameter, which ultimately
resulted in overload confusion and broken NLog tests + the introduction
of a "kill switch" env variable to disable the functionality during
debugging.

The fact that the overload selection was confusing indicates to me that
we should _not_ be using optional parameters here, otherwise we're just
asking for the same problem to happen again. Similarly, having the kill
switch seems dangerous, as AFAICT there's basically no scenarios where
we would want to disable this functionality globally, as it would just
break any integration that relied on it.

## Implementation details

- Add additional overloads for the common `CallTargetState` constructors
to avoid hitting the optional parameter `skipMethodBody`. It means that
if you want to set `skipMethodBody=true`, you have to pass an additional
`state` argument too, but that seems a small price to pay for the high
level of confusion otherwise
- ~Remove the `DD_INTERNAL_SKIP_METHOD_BODY_ENABLED` kill-switch key~
After further discussion, reinstated this given the native changes carry
some risk. We'll remove this as soon as we have an integration that uses
the functionality, or potentially just in the next release either way

## Test coverage

As long as all the existing tests pass, we're ok
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants