Skip to content
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

Add an IsManualInstrumentationOnly flag to Datadog.Trace.Manual #5866

Merged
merged 5 commits into from
Aug 13, 2024

Conversation

andrewlock
Copy link
Member

@andrewlock andrewlock commented Aug 8, 2024

Summary of changes

Adds a IsManualInstrumentationOnly() method to the manual library, and rewrites it when we're doing automatic instrumentation

Reason for change

For some scenarios we're considering (e.g. v3 part deux) we need to know from inside the manual library whether we're auto-instrumenting. This is a relatively simple approach to achieve it.

Implementation details

Mostly copied C++ rewriting code that we're already doing, and tweaked it for our use case

Test coverage

Added a check to the manualinstrumentation tests that the flag behaves as expected (true when no auto instrumentation, false if we have auto instrumentation)

Other details

For part deux we'll need to do more rewriting, but there shouldn't be any harm in doing the rewriting in this PR regardless of what happens with that

@andrewlock andrewlock added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) labels Aug 8, 2024
@andrewlock andrewlock requested a review from a team as a code owner August 8, 2024 14:48
@andrewlock andrewlock changed the title Add an IsManualInstrumentationOnly flag to _Datadog.Trace.Manual_ Add an IsManualInstrumentationOnly flag to Datadog.Trace.Manual Aug 8, 2024
@andrewlock
Copy link
Member Author

andrewlock commented Aug 8, 2024

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 (5866) - mean (72ms)  : 64, 80
     .   : milestone, 72,
    master - mean (72ms)  : 64, 79
     .   : milestone, 72,

    section CallTarget+Inlining+NGEN
    This PR (5866) - mean (1,064ms)  : 1038, 1090
     .   : milestone, 1064,
    master - mean (1,072ms)  : 1045, 1098
     .   : milestone, 1072,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5866) - mean (109ms)  : 104, 114
     .   : milestone, 109,
    master - mean (109ms)  : 105, 113
     .   : milestone, 109,

    section CallTarget+Inlining+NGEN
    This PR (5866) - mean (869ms)  : crit, 601, 1137
     .   : crit, milestone, 869,
    master - mean (747ms)  : 729, 764
     .   : milestone, 747,

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

    section CallTarget+Inlining+NGEN
    This PR (5866) - mean (715ms)  : 667, 762
     .   : milestone, 715,
    master - mean (704ms)  : 677, 730
     .   : milestone, 704,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5866) - mean (192ms)  : 189, 196
     .   : milestone, 192,
    master - mean (192ms)  : 189, 194
     .   : milestone, 192,

    section CallTarget+Inlining+NGEN
    This PR (5866) - mean (1,172ms)  : 1140, 1203
     .   : milestone, 1172,
    master - mean (1,169ms)  : 1144, 1195
     .   : milestone, 1169,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5866) - mean (278ms)  : 273, 282
     .   : milestone, 278,
    master - mean (276ms)  : 272, 281
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (5866) - mean (923ms)  : 896, 949
     .   : milestone, 923,
    master - mean (914ms)  : 896, 933
     .   : milestone, 914,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5866) - mean (267ms)  : 262, 272
     .   : milestone, 267,
    master - mean (265ms)  : 260, 270
     .   : milestone, 265,

    section CallTarget+Inlining+NGEN
    This PR (5866) - mean (910ms)  : 887, 932
     .   : milestone, 910,
    master - mean (901ms)  : 879, 922
     .   : milestone, 901,

Loading

@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Aug 8, 2024

Datadog Report

Branch report: andrew/add-is-auto-enabled-flag
Commit report: 8195107
Test service: dd-trace-dotnet

✅ 0 Failed, 347700 Passed, 2258 Skipped, 23h 42m 26.24s Total Time

@andrewlock
Copy link
Member Author

andrewlock commented Aug 8, 2024

Benchmarks Report for tracer 🐌

Benchmarks for #5866 compared to master:

  • 1 benchmarks are slower, with geometric mean 1.155
  • 1 benchmarks have more 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 - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5866

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.ActivityBenchmark.StartStopWithChild‑net472 1.155 16,058.18 18,539.80

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 7.74μs 44.4ns 347ns 0.0156 0.00781 0 5.43 KB
master StartStopWithChild netcoreapp3.1 9.93μs 56.1ns 401ns 0.0241 0.00966 0 5.62 KB
master StartStopWithChild net472 16μs 52.5ns 203ns 1.02 0.304 0.0881 6.05 KB
#5866 StartStopWithChild net6.0 7.79μs 43.9ns 314ns 0.0148 0.00738 0 5.43 KB
#5866 StartStopWithChild netcoreapp3.1 9.77μs 53.4ns 311ns 0.0199 0.00993 0 5.62 KB
#5866 StartStopWithChild net472 18.5μs 87.5ns 350ns 1.03 0.32 0.101 6.07 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 471μs 393ns 1.52μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 638μs 254ns 982ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 831μs 1.99μs 7.44μs 0.408 0 0 3.3 KB
#5866 WriteAndFlushEnrichedTraces net6.0 468μs 457ns 1.71μs 0 0 0 2.7 KB
#5866 WriteAndFlushEnrichedTraces netcoreapp3.1 644μs 641ns 2.4μs 0 0 0 2.7 KB
#5866 WriteAndFlushEnrichedTraces net472 839μs 436ns 1.69μs 0.417 0 0 3.3 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 193μs 1.1μs 7.79μs 0.189 0 0 18.45 KB
master SendRequest netcoreapp3.1 217μs 1.24μs 8.96μs 0.203 0 0 20.61 KB
master SendRequest net472 0.00159ns 0.000469ns 0.00182ns 0 0 0 0 b
#5866 SendRequest net6.0 197μs 1.08μs 6μs 0.197 0 0 18.45 KB
#5866 SendRequest netcoreapp3.1 220μs 1.24μs 8.76μs 0.224 0 0 20.61 KB
#5866 SendRequest net472 0.00438ns 0.00223ns 0.00833ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #5866

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑net6.0 41.52 KB 41.79 KB 274 B 0.66%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 564μs 1.45μs 5.42μs 0.558 0 0 41.52 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 668μs 1.57μs 5.67μs 0.331 0 0 41.7 KB
master WriteAndFlushEnrichedTraces net472 866μs 3.87μs 15μs 8.36 2.64 0.44 53.3 KB
#5866 WriteAndFlushEnrichedTraces net6.0 590μs 3.31μs 21.7μs 0.587 0 0 41.79 KB
#5866 WriteAndFlushEnrichedTraces netcoreapp3.1 698μs 3.85μs 36.3μs 0.34 0 0 41.71 KB
#5866 WriteAndFlushEnrichedTraces net472 852μs 3.86μs 14.9μs 8.33 2.5 0.417 53.3 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.34μs 0.907ns 3.39ns 0.0141 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.75μs 2.5ns 9.68ns 0.0132 0 0 1.02 KB
master ExecuteNonQuery net472 2.02μs 1.99ns 7.71ns 0.157 0 0 987 B
#5866 ExecuteNonQuery net6.0 1.26μs 1.68ns 6.52ns 0.0144 0 0 1.02 KB
#5866 ExecuteNonQuery netcoreapp3.1 1.69μs 1.28ns 4.97ns 0.0137 0 0 1.02 KB
#5866 ExecuteNonQuery net472 1.98μs 1.33ns 4.96ns 0.156 0 0 987 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.18μs 0.486ns 1.88ns 0.0137 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.48μs 0.567ns 2.12ns 0.0133 0 0 976 B
master CallElasticsearch net472 2.47μs 1.89ns 7.3ns 0.158 0 0 995 B
master CallElasticsearchAsync net6.0 1.33μs 1.33ns 5.16ns 0.0133 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.69μs 2.4ns 8.99ns 0.0135 0 0 1.02 KB
master CallElasticsearchAsync net472 2.75μs 1.78ns 6.91ns 0.166 0 0 1.05 KB
#5866 CallElasticsearch net6.0 1.27μs 0.997ns 3.86ns 0.0134 0 0 976 B
#5866 CallElasticsearch netcoreapp3.1 1.48μs 0.871ns 3.26ns 0.0131 0 0 976 B
#5866 CallElasticsearch net472 2.41μs 1.73ns 5.98ns 0.157 0 0 995 B
#5866 CallElasticsearchAsync net6.0 1.27μs 0.483ns 1.81ns 0.0134 0 0 952 B
#5866 CallElasticsearchAsync netcoreapp3.1 1.71μs 0.993ns 3.71ns 0.0137 0 0 1.02 KB
#5866 CallElasticsearchAsync net472 2.64μs 1.97ns 7.61ns 0.167 0 0 1.05 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.22μs 1.85ns 7.17ns 0.0134 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.65μs 1.26ns 4.88ns 0.0122 0 0 952 B
master ExecuteAsync net472 1.75μs 0.941ns 3.52ns 0.145 0 0 915 B
#5866 ExecuteAsync net6.0 1.27μs 1.13ns 4.22ns 0.0134 0 0 952 B
#5866 ExecuteAsync netcoreapp3.1 1.65μs 2.35ns 9.12ns 0.0124 0 0 952 B
#5866 ExecuteAsync net472 1.75μs 0.875ns 3.39ns 0.145 0 0 915 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.04μs 1.88ns 7.29ns 0.0303 0 0 2.22 KB
master SendAsync netcoreapp3.1 5.14μs 2.2ns 8.52ns 0.036 0 0 2.76 KB
master SendAsync net472 7.72μs 1.68ns 6.51ns 0.499 0 0 3.15 KB
#5866 SendAsync net6.0 4.11μs 3.02ns 11.7ns 0.0309 0 0 2.22 KB
#5866 SendAsync netcoreapp3.1 5.19μs 1.26ns 4.88ns 0.0362 0 0 2.76 KB
#5866 SendAsync net472 7.65μs 2.55ns 9.89ns 0.498 0 0 3.15 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.46μs 0.753ns 2.92ns 0.0234 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.31μs 1.68ns 6.5ns 0.0222 0 0 1.64 KB
master EnrichedLog net472 2.73μs 1.28ns 4.61ns 0.249 0 0 1.57 KB
#5866 EnrichedLog net6.0 1.45μs 1.76ns 6.6ns 0.0231 0 0 1.64 KB
#5866 EnrichedLog netcoreapp3.1 2.17μs 1.03ns 3.57ns 0.0217 0 0 1.64 KB
#5866 EnrichedLog net472 2.64μs 4.59ns 17.8ns 0.249 0 0 1.57 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 117μs 185ns 717ns 0.0587 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 119μs 234ns 907ns 0.06 0 0 4.28 KB
master EnrichedLog net472 149μs 180ns 697ns 0.664 0.221 0 4.46 KB
#5866 EnrichedLog net6.0 116μs 103ns 400ns 0.058 0 0 4.28 KB
#5866 EnrichedLog netcoreapp3.1 120μs 261ns 1.01μs 0.0592 0 0 4.28 KB
#5866 EnrichedLog net472 149μs 178ns 690ns 0.672 0.224 0 4.46 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 3.06μs 1.12ns 4.35ns 0.0306 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.13μs 1.77ns 6.86ns 0.0289 0 0 2.2 KB
master EnrichedLog net472 4.73μs 1.38ns 5.33ns 0.319 0 0 2.02 KB
#5866 EnrichedLog net6.0 3.16μs 7.91ns 30.6ns 0.0311 0 0 2.2 KB
#5866 EnrichedLog netcoreapp3.1 4.17μs 0.828ns 3.1ns 0.0293 0 0 2.2 KB
#5866 EnrichedLog net472 4.8μs 3.53ns 13.7ns 0.32 0 0 2.02 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.41μs 1.24ns 4.82ns 0.0163 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.8μs 0.656ns 2.37ns 0.0154 0 0 1.14 KB
master SendReceive net472 2.03μs 0.892ns 3.45ns 0.183 0.00103 0 1.16 KB
#5866 SendReceive net6.0 1.37μs 0.619ns 2.4ns 0.0156 0 0 1.14 KB
#5866 SendReceive netcoreapp3.1 1.69μs 1.19ns 4.46ns 0.0151 0 0 1.14 KB
#5866 SendReceive net472 2.16μs 0.575ns 2.15ns 0.184 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.77μs 0.798ns 3.09ns 0.0222 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.91μs 2.22ns 8.59ns 0.0217 0 0 1.65 KB
master EnrichedLog net472 4.44μs 3.59ns 13.9ns 0.323 0 0 2.04 KB
#5866 EnrichedLog net6.0 2.85μs 0.998ns 3.6ns 0.0214 0 0 1.6 KB
#5866 EnrichedLog netcoreapp3.1 3.9μs 3.54ns 13.7ns 0.0217 0 0 1.65 KB
#5866 EnrichedLog net472 4.48μs 2.35ns 8.78ns 0.322 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 403ns 0.177ns 0.685ns 0.00813 0 0 576 B
master StartFinishSpan netcoreapp3.1 604ns 0.245ns 0.915ns 0.00757 0 0 576 B
master StartFinishSpan net472 621ns 0.514ns 1.92ns 0.0915 0 0 578 B
master StartFinishScope net6.0 484ns 0.338ns 1.31ns 0.00988 0 0 696 B
master StartFinishScope netcoreapp3.1 700ns 0.385ns 1.49ns 0.0095 0 0 696 B
master StartFinishScope net472 860ns 0.846ns 3.17ns 0.104 0 0 658 B
#5866 StartFinishSpan net6.0 403ns 0.955ns 3.7ns 0.00802 0 0 576 B
#5866 StartFinishSpan netcoreapp3.1 560ns 1.37ns 5.11ns 0.00782 0 0 576 B
#5866 StartFinishSpan net472 633ns 0.541ns 2.09ns 0.0916 0 0 578 B
#5866 StartFinishScope net6.0 488ns 0.201ns 0.778ns 0.00972 0 0 696 B
#5866 StartFinishScope netcoreapp3.1 663ns 0.412ns 1.6ns 0.00924 0 0 696 B
#5866 StartFinishScope net472 841ns 0.938ns 3.63ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 586ns 0.212ns 0.821ns 0.00974 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 916ns 4.53ns 19.7ns 0.00931 0 0 696 B
master RunOnMethodBegin net472 1.12μs 0.419ns 1.57ns 0.104 0 0 658 B
#5866 RunOnMethodBegin net6.0 622ns 1.92ns 7.17ns 0.00973 0 0 696 B
#5866 RunOnMethodBegin netcoreapp3.1 951ns 0.917ns 3.55ns 0.0092 0 0 696 B
#5866 RunOnMethodBegin net472 1.12μs 0.479ns 1.86ns 0.104 0 0 658 B

Comment on lines +961 to +962
// If/when we go with v3 part deux, we will need to update this to
// also rewrite to support version mismatch via IDistributedTracer
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not actually sure this is needed.

Automatic v3.deux with manual v2: we already rewrite DistributedTracer in the manual Datadog.Trace
Automatic v2 with manual v3.deux: it won't work anyway because v2 doesn't know it has to rewrite Datadog.Trace.Manual

Copy link
Member Author

Choose a reason for hiding this comment

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

it won't work anyway because v2 doesn't know it has to rewrite Datadog.Trace.Manual

Maybe it should :stare:

Copy link
Member

@lucaspimentel lucaspimentel Aug 9, 2024

Choose a reason for hiding this comment

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

Do we want to support v2 + v3 (in either combination of auto+manual)? Wouldn't it be simpler to only support v3+v3? I thought with the new way we do manual by rewriting we were getting rid of DistributedTracer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to support v2 + v3

There's a difference between "support" and "have migration path".

If we categorically don't work with v2 + v3 then customers are once again stuck having to update their dev dependency (manual) and their automatic version (infra) in exact lockstep. We know from experience this is hard, and customers won't do it.

If we want customers to be able to migrate easily to v3, then I'm inclined to say we should try to make it work if we can (and we probably can initially at least). We still wouldn't support it though - i.e. first thing we do in support is say "this is not supported" and make them upgrade.

I thought with the new way we do manual by rewriting we were getting rid of DistributedTracer.

That has not been on the cards, as it would properly break things if they mix versions I believe 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

But anyway, that's moot for now, as it's only required if we put the PoC into action, which isn't clear at this point. We can decide how to handle it down the line, and put out another v2 release which adds compatibility if necessary

tracer/src/Datadog.Tracer.Native/cor_profiler.cpp Outdated Show resolved Hide resolved
mdTokenNil, &instrumentationTypeDef);
if (FAILED(hr))
{
Logger::Warn("Error rewriting IsManualInstrumentationOnly on getting Instrumentation TypeDef");
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be an error? If this fails (type / method not found) something nasty must have happened...

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably 🤷‍♂️ But

  • It's going to be logged in both cases anyway
  • Neither one ends up in telemetry logs
  • It doesn't break the overall tracer, just the manual instrumentation
  • It's what we use for other rewriting currently

So, meh? 😃

// to this:
// IL_0000: ldc.i4.0
// IL_0001: ret
ILRewriterWrapper wrapper(&methodRewriter);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT. I'd look for the instruction, rather that blindly trusting the ldc.i4.1 is going to be the first one

Copy link
Member Author

@andrewlock andrewlock Aug 12, 2024

Choose a reason for hiding this comment

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

Maybe I've misunderstood, but I thought that I was specifying the exact commands here rather than anything else? i.e. it doesn't matter what was there before does it, I'm just specifying the "new" body I thought? (confirmed with @kevingosse that's the case - we're not importing the body at all)

Copy link
Contributor

@daniel-romano-DD daniel-romano-DD left a comment

Choose a reason for hiding this comment

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

LGTM with a minor comment. TY

@andrewlock andrewlock merged commit 17a46a7 into master Aug 13, 2024
67 of 71 checks passed
@andrewlock andrewlock deleted the andrew/add-is-auto-enabled-flag branch August 13, 2024 08:36
@github-actions github-actions bot added this to the vNext-v3 milestone Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants