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.

Sign up for GitHub

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

Jump to bottom

Prevent NullReferenceException when Activity IDs are null #5690

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Prevent NullReferenceException when Activity IDs are null #5690

merged 1 commit into from
Jun 14, 2024

Conversation

bouwkast
Copy link
Contributor

Summary of changes

Error Tracking identified instances where Activity objects were being handled by DefaultActivityHandler where they lacked any Activity.Id and Activity.TraceId/Activity.SpanId.

This now checks if the generated key used for mapping Activity -> Scope is null and if so skip it for both Start and Stop. Note that only the Start` method was in Error Tracking.

Reason for change

Null Reference Exception was being thrown in the GetOrAdd since the key was null

Implementation details

Check key for null, skip if so for both Start/Stop

Test coverage

Struggled replicating this. Also struggled figuring out how this is happening reading through source code for Activity

Other details

@github-actions github-actions bot added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Jun 13, 2024
bouwkast
bouwkast commented Jun 13, 2024
View reviewed changes
tracer/src/Datadog.Trace/Activity/Handlers/ActivityHandlerCommon.cs
// unsure how exactly this occurs after reading through the Activity source code
// Activity.Id, Activity.SpanId and/or Activity.TraceId were null
// if this is the case, just ignore the Activity
activityMapping = default;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on adding logging here of some sort?

Warning level?

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on adding logging here of some sort

Maybe. My concern is that we don't understand why this happens, so we'll be writing a lot of logs, which noone will see 😬

@bouwkast bouwkast marked this pull request as ready for review June 13, 2024 20:32
@bouwkast bouwkast requested a review from a team as a code owner June 13, 2024 20:32
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Jun 13, 2024 •
edited
Loading

Datadog Report

Branch report: steven/null-activity-id
Commit report: 4068b2e
Test service: dd-trace-dotnet

✅ 0 Failed, 337754 Passed, 1654 Skipped, 14h 8m 0.33s Total Time

@andrewlock
Copy link
Member

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 (5690) - mean (73ms)  : 64, 82
     .   : milestone, 73,
    master - mean (75ms)  : 65, 84
     .   : milestone, 75,

    section CallTarget+Inlining+NGEN
    This PR (5690) - mean (1,013ms)  : 991, 1036
     .   : milestone, 1013,
    master - mean (1,009ms)  : 989, 1029
     .   : milestone, 1009,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5690) - mean (110ms)  : 106, 113
     .   : milestone, 110,
    master - mean (109ms)  : 105, 113
     .   : milestone, 109,

    section CallTarget+Inlining+NGEN
    This PR (5690) - mean (727ms)  : 706, 748
     .   : milestone, 727,
    master - mean (726ms)  : 705, 747
     .   : milestone, 726,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5690) - mean (93ms)  : 90, 95
     .   : milestone, 93,
    master - mean (93ms)  : 90, 96
     .   : milestone, 93,

    section CallTarget+Inlining+NGEN
    This PR (5690) - mean (682ms)  : 663, 700
     .   : milestone, 682,
    master - mean (682ms)  : 659, 705
     .   : milestone, 682,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5690) - mean (192ms)  : 188, 196
     .   : milestone, 192,
    master - mean (194ms)  : 189, 200
     .   : milestone, 194,

    section CallTarget+Inlining+NGEN
    This PR (5690) - mean (1,114ms)  : 1097, 1131
     .   : milestone, 1114,
    master - mean (1,118ms)  : 1095, 1140
     .   : milestone, 1118,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5690) - mean (278ms)  : 273, 283
     .   : milestone, 278,
    master - mean (279ms)  : 273, 286
     .   : milestone, 279,

    section CallTarget+Inlining+NGEN
    This PR (5690) - mean (922ms)  : 902, 942
     .   : milestone, 922,
    master - mean (914ms)  : 891, 938
     .   : milestone, 914,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5690) - mean (268ms)  : 261, 276
     .   : milestone, 268,
    master - mean (269ms)  : 262, 275
     .   : milestone, 269,

    section CallTarget+Inlining+NGEN
    This PR (5690) - mean (900ms)  : 881, 920
     .   : milestone, 900,
    master - mean (901ms)  : 878, 923
     .   : milestone, 901,
Loading

@andrewlock
Copy link
Member

Throughput/Crank Report:zap:

Throughput results for AspNetCoreSimpleController comparing the following branches/commits:

Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red.

Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards!

gantt
    title Throughput Linux x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5690) (11.542M)   : 0, 11542013
    master (11.523M)   : 0, 11522854
    benchmarks/2.9.0 (11.535M)   : 0, 11535406

    section Automatic
    This PR (5690) (7.714M)   : 0, 7714167
    master (7.723M)   : 0, 7722745
    benchmarks/2.9.0 (8.170M)   : 0, 8170289

    section Trace stats
    master (8.131M)   : 0, 8131334

    section Manual
    This PR (5690) (9.883M)   : 0, 9883003
    master (9.819M)   : 0, 9819349

    section Manual + Automatic
    This PR (5690) (7.292M)   : 0, 7291590
    master (7.371M)   : 0, 7371256

    section Version Conflict
    master (6.536M)   : 0, 6536186
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5690) (9.668M)   : 0, 9667824
    master (9.532M)   : 0, 9532110
    benchmarks/2.9.0 (9.561M)   : 0, 9560775

    section Automatic
    This PR (5690) (6.522M)   : 0, 6521816
    master (6.533M)   : 0, 6532879

    section Trace stats
    master (6.849M)   : 0, 6848972

    section Manual
    This PR (5690) (8.241M)   : 0, 8240631
    master (8.068M)   : 0, 8068367

    section Manual + Automatic
    This PR (5690) (6.327M)   : 0, 6327192
    master (6.066M)   : 0, 6066100

    section Version Conflict
    master (5.612M)   : 0, 5611591
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5690) (10.101M)   : 0, 10101130
    master (10.059M)   : 0, 10059074
    benchmarks/2.9.0 (10.236M)   : 0, 10235800

    section Automatic
    This PR (5690) (6.853M)   : 0, 6852852
    master (7.175M)   : 0, 7174979
    benchmarks/2.9.0 (7.335M)   : 0, 7335026

    section Trace stats
    master (7.345M)   : 0, 7344900

    section Manual
    This PR (5690) (8.848M)   : 0, 8848366
    master (8.705M)   : 0, 8705451

    section Manual + Automatic
    This PR (5690) (6.932M)   : 0, 6931965
    master (6.832M)   : 0, 6831957

    section Version Conflict
    master (6.198M)   : 0, 6198444
Loading

@andrewlock
Copy link
Member

Benchmarks Report for tracer 🐌

Benchmarks for #5690 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.188
  • 1 benchmarks are slower, with geometric mean 1.124
  • 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 ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 8.33μs 79.8ns 798ns 0.0225 0.00899 0 5.42 KB
master StartStopWithChild netcoreapp3.1 9.64μs 52.9ns 330ns 0.0184 0.0092 0 5.62 KB
master StartStopWithChild net472 16μs 41.6ns 156ns 1.02 0.314 0.0909 6.07 KB
#5690 StartStopWithChild net6.0 7.88μs 41.6ns 303ns 0.0198 0.0079 0 5.43 KB
#5690 StartStopWithChild netcoreapp3.1 10.1μs 53.8ns 285ns 0.0242 0.00968 0 5.62 KB
#5690 StartStopWithChild net472 16.2μs 54.1ns 210ns 1.01 0.293 0.0813 6.06 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 473μs 188ns 729ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 630μs 372ns 1.39μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 834μs 362ns 1.35μs 0.414 0 0 3.3 KB
#5690 WriteAndFlushEnrichedTraces net6.0 473μs 201ns 751ns 0 0 0 2.7 KB
#5690 WriteAndFlushEnrichedTraces netcoreapp3.1 616μs 256ns 992ns 0 0 0 2.7 KB
#5690 WriteAndFlushEnrichedTraces net472 824μs 229ns 888ns 0.408 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 172μs 201ns 780ns 0.255 0 0 18.44 KB
master SendRequest netcoreapp3.1 192μs 389ns 1.51μs 0.191 0 0 20.6 KB
master SendRequest net472 0.000279ns 0.000153ns 0.000591ns 0 0 0 0 b
#5690 SendRequest net6.0 172μs 278ns 1.08μs 0.258 0 0 18.44 KB
#5690 SendRequest netcoreapp3.1 193μs 284ns 1.1μs 0.192 0 0 20.6 KB
#5690 SendRequest net472 1.98E‑05ns 1.98E‑05ns 7.66E‑05ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #5690

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑netcoreapp3.1 41.89 KB 41.56 KB -334 B -0.80%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 543μs 383ns 1.48μs 0.548 0 0 41.6 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 661μs 1.01μs 3.92μs 0.322 0 0 41.89 KB
master WriteAndFlushEnrichedTraces net472 879μs 3.77μs 14.6μs 8.19 2.59 0.431 53.23 KB
#5690 WriteAndFlushEnrichedTraces net6.0 549μs 881ns 3.41μs 0.546 0 0 41.63 KB
#5690 WriteAndFlushEnrichedTraces netcoreapp3.1 642μs 1.36μs 5.27μs 0.322 0 0 41.56 KB
#5690 WriteAndFlushEnrichedTraces net472 867μs 3.61μs 14μs 8.3 2.62 0.437 53.25 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.12μs 0.416ns 1.61ns 0.0114 0 0 808 B
master ExecuteNonQuery netcoreapp3.1 1.48μs 0.794ns 3.08ns 0.0111 0 0 808 B
master ExecuteNonQuery net472 1.76μs 1.79ns 6.93ns 0.122 0 0 770 B
#5690 ExecuteNonQuery net6.0 1.05μs 0.426ns 1.65ns 0.0112 0 0 808 B
#5690 ExecuteNonQuery netcoreapp3.1 1.45μs 1.2ns 4.64ns 0.0109 0 0 808 B
#5690 ExecuteNonQuery net472 1.71μs 3ns 11.6ns 0.122 0 0 770 B
Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5690

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync‑net6.0 1.124 1,149.67 1,291.94

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.23μs 1.04ns 4.01ns 0.0135 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.54μs 2.11ns 7.9ns 0.0134 0 0 976 B
master CallElasticsearch net472 2.51μs 1.44ns 5.38ns 0.158 0 0 995 B
master CallElasticsearchAsync net6.0 1.15μs 0.402ns 1.45ns 0.0132 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.65μs 0.746ns 2.79ns 0.0141 0 0 1.02 KB
master CallElasticsearchAsync net472 2.59μs 2.41ns 9.35ns 0.167 0 0 1.05 KB
#5690 CallElasticsearch net6.0 1.32μs 0.855ns 3.2ns 0.0139 0 0 976 B
#5690 CallElasticsearch netcoreapp3.1 1.56μs 2.43ns 9.42ns 0.013 0 0 976 B
#5690 CallElasticsearch net472 2.6μs 3.9ns 15.1ns 0.158 0 0 995 B
#5690 CallElasticsearchAsync net6.0 1.29μs 0.686ns 2.57ns 0.0129 0 0 952 B
#5690 CallElasticsearchAsync netcoreapp3.1 1.66μs 1.43ns 5.34ns 0.0139 0 0 1.02 KB
#5690 CallElasticsearchAsync net472 2.56μs 2.61ns 9.42ns 0.166 0.00127 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.33μs 0.501ns 1.88ns 0.0133 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.51μs 0.738ns 2.86ns 0.0128 0 0 952 B
master ExecuteAsync net472 1.86μs 0.838ns 3.14ns 0.145 0 0 915 B
#5690 ExecuteAsync net6.0 1.22μs 1.26ns 4.87ns 0.0134 0 0 952 B
#5690 ExecuteAsync netcoreapp3.1 1.63μs 4.7ns 18.2ns 0.0122 0 0 952 B
#5690 ExecuteAsync net472 1.84μ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.01μs 1.35ns 5.21ns 0.03 0 0 2.22 KB
master SendAsync netcoreapp3.1 4.97μs 6.28ns 23.5ns 0.0374 0 0 2.76 KB
master SendAsync net472 7.89μs 2.35ns 9.12ns 0.498 0 0 3.15 KB
#5690 SendAsync net6.0 4.21μs 8.73ns 33.8ns 0.0314 0 0 2.22 KB
#5690 SendAsync netcoreapp3.1 5.1μs 12.5ns 48.6ns 0.0353 0 0 2.76 KB
#5690 SendAsync net472 7.76μs 2.36ns 8.83ns 0.496 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.44μs 0.712ns 2.66ns 0.0234 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.29μs 1.36ns 5.09ns 0.0224 0 0 1.64 KB
master EnrichedLog net472 2.51μs 1.61ns 6.24ns 0.249 0 0 1.57 KB
#5690 EnrichedLog net6.0 1.58μs 0.647ns 2.42ns 0.023 0 0 1.64 KB
#5690 EnrichedLog netcoreapp3.1 2.1μs 1.02ns 3.68ns 0.0221 0 0 1.64 KB
#5690 EnrichedLog net472 2.46μs 1.99ns 7.45ns 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 113μs 159ns 617ns 0.0566 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 118μs 171ns 641ns 0 0 0 4.28 KB
master EnrichedLog net472 149μs 126ns 490ns 0.672 0.224 0 4.46 KB
#5690 EnrichedLog net6.0 112μs 145ns 541ns 0.0562 0 0 4.28 KB
#5690 EnrichedLog netcoreapp3.1 119μs 229ns 888ns 0 0 0 4.28 KB
#5690 EnrichedLog net472 147μs 115ns 445ns 0.664 0.221 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.16μs 0.98ns 3.67ns 0.0302 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.22μs 1.5ns 5.4ns 0.0296 0 0 2.2 KB
master EnrichedLog net472 4.8μs 1.82ns 7.04ns 0.318 0 0 2.02 KB
#5690 EnrichedLog net6.0 3.08μs 0.934ns 3.62ns 0.0308 0 0 2.2 KB
#5690 EnrichedLog netcoreapp3.1 4.04μs 1.16ns 4.34ns 0.03 0 0 2.2 KB
#5690 EnrichedLog net472 5.16μs 1.41ns 5.45ns 0.319 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.31μs 0.852ns 3.3ns 0.0158 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.78μs 0.894ns 3.22ns 0.0151 0 0 1.14 KB
master SendReceive net472 2.07μs 1.02ns 3.95ns 0.183 0 0 1.16 KB
#5690 SendReceive net6.0 1.33μs 1.01ns 3.91ns 0.0161 0 0 1.14 KB
#5690 SendReceive netcoreapp3.1 1.77μs 0.533ns 2.06ns 0.0149 0 0 1.14 KB
#5690 SendReceive net472 2.03μs 0.987ns 3.82ns 0.183 0.00101 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.85μs 0.479ns 1.86ns 0.0214 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 4.01μs 1.88ns 7.27ns 0.022 0 0 1.65 KB
master EnrichedLog net472 4.5μs 3.24ns 12.6ns 0.323 0 0 2.04 KB
#5690 EnrichedLog net6.0 2.82μs 2.15ns 8.04ns 0.0224 0 0 1.6 KB
#5690 EnrichedLog netcoreapp3.1 4.01μs 2.62ns 10.1ns 0.0219 0 0 1.65 KB
#5690 EnrichedLog net472 4.4μs 10.1ns 39.1ns 0.322 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #5690

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 1.188 559.46 470.99

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 393ns 0.107ns 0.402ns 0.00812 0 0 576 B
master StartFinishSpan netcoreapp3.1 541ns 0.284ns 1.1ns 0.0078 0 0 576 B
master StartFinishSpan net472 674ns 0.546ns 2.11ns 0.0916 0 0 578 B
master StartFinishScope net6.0 560ns 0.209ns 0.783ns 0.00973 0 0 696 B
master StartFinishScope netcoreapp3.1 744ns 0.252ns 0.978ns 0.00933 0 0 696 B
master StartFinishScope net472 844ns 0.366ns 1.42ns 0.104 0 0 658 B
#5690 StartFinishSpan net6.0 395ns 0.129ns 0.483ns 0.00795 0 0 576 B
#5690 StartFinishSpan netcoreapp3.1 563ns 0.258ns 0.931ns 0.00787 0 0 576 B
#5690 StartFinishSpan net472 626ns 0.306ns 1.19ns 0.0916 0 0 578 B
#5690 StartFinishScope net6.0 471ns 0.227ns 0.88ns 0.00973 0 0 696 B
#5690 StartFinishScope netcoreapp3.1 735ns 0.584ns 2.02ns 0.00953 0 0 696 B
#5690 StartFinishScope net472 856ns 0.922ns 3.57ns 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 653ns 0.288ns 1.12ns 0.00975 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 924ns 0.212ns 0.82ns 0.00931 0 0 696 B
master RunOnMethodBegin net472 1.06μs 0.24ns 0.899ns 0.104 0 0 658 B
#5690 RunOnMethodBegin net6.0 631ns 0.194ns 0.753ns 0.00982 0 0 696 B
#5690 RunOnMethodBegin netcoreapp3.1 904ns 0.303ns 1.17ns 0.0096 0 0 696 B
#5690 RunOnMethodBegin net472 1.11μs 0.738ns 2.86ns 0.104 0 0 658 B

andrewlock
andrewlock approved these changes Jun 14, 2024
View reviewed changes
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.

Thanks!

@andrewlock andrewlock added the area:opentelemetry OpenTelemetry support label Jun 14, 2024
@bouwkast bouwkast merged commit 38ec6c9 into master Jun 14, 2024
57 checks passed
@bouwkast bouwkast deleted the steven/null-activity-id branch June 14, 2024 17:45
@github-actions github-actions bot added this to the vNext-v2 milestone Jun 14, 2024
andrewlock pushed a commit that referenced this pull request Jun 17, 2024
@bouwkast @andrewlock
Skip Activity if there are no Ids (#5690)
688f554
@andrewlock andrewlock modified the milestones: vNext-v2, 2.53.1 Jun 17, 2024
@bouwkast bouwkast mentioned this pull request Jun 25, 2024
Merged
bouwkast added a commit that referenced this pull request Jul 2, 2024
@bouwkast
Do not reset Activity.Id for non-W3C formats (#5739)
b8c6db0 
## Summary of changes

This makes it so that we validate that the `Activity` object is in W3C
format (by checking that the `Activity` has a SpanID and TraceID after
it stars) if we are updating the SpanId and TraceId.

Note that for this to happen the `Activity` needs to be a non-W3C format
_and_ it's start time must be less than the Datadog Active Span start
time. This start time issue can happen as an `Activity` can modify its
start time _after_ it is started and we don't update the created Datadog
span with that new information.

## Reason for change

T fixes an edge case where an `Activity` object could be in a non-W3C
format and hit this code resulting in its `Id` being erroneously set to
`null` causing a `NullReferenceException` for us (workaround in #5690),
but could have also thrown if anyone accessed that `Id` and attempted to
make a call on it.

## Implementation details

Check to ensure that the `Activity` is in W3C format - this is done by
just checking that it already has a Span Id and Trace Id.

## Test coverage

Added tests to `Samples.NetActivitySdk`, the
`RunManuallyUpdatedStartTime()` would fail and throw an exception.

## Other details

Should consider handling instances where users update the start time of
the `Activity` and update the Datadog `Span` accordingly.

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes

@andrewlock andrewlock andrewlock approved these changes

Assignees
No one assigned
Labels
area:opentelemetry OpenTelemetry support area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) identified-by:telemetry
Projects
None yet
Milestone
2.53.1
Development

Successfully merging this pull request may close these issues.
3 participants
@bouwkast @andrewlock @zacharycmontoya