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

[Tracer] Update signature parsing in the tracer's native library to account for ELEMENT_TYPE_PTR byte #5042

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

zacharycmontoya
Copy link
Collaborator

@zacharycmontoya zacharycmontoya commented Jan 10, 2024

Summary of changes

Reason for change

Implementation details

Test coverage

Added minimal unit tests to cover the new scenarios.

Other details

We haven't seen any issues in our current usage, but this bug fix was required for the opentelemetry/opentelemetry-dotnet-instrumentation project to implement their initial version of their continuous profiler.

…TYPE_PTR byte. Added minimal unit tests as regression tests
@zacharycmontoya zacharycmontoya self-assigned this Jan 10, 2024
@zacharycmontoya zacharycmontoya requested a review from a team as a code owner January 10, 2024 23:53
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Jan 11, 2024

Datadog Report

Branch report: zach/profiler-sig-token-fix
Commit report: 6c66504
Test service: dd-trace-dotnet

✅ 0 Failed, 306771 Passed, 1463 Skipped, 31m 34.45s Wall 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).

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5042) - mean (71ms)  : 64, 79
     .   : milestone, 71,
    master - mean (69ms)  : 61, 77
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (5042) - mean (1,024ms)  : 1005, 1043
     .   : milestone, 1024,
    master - mean (1,027ms)  : 1010, 1044
     .   : milestone, 1027,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5042) - mean (105ms)  : 100, 110
     .   : milestone, 105,
    master - mean (105ms)  : 99, 111
     .   : milestone, 105,

    section CallTarget+Inlining+NGEN
    This PR (5042) - mean (737ms)  : 712, 762
     .   : milestone, 737,
    master - mean (734ms)  : 715, 753
     .   : milestone, 734,

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

    section CallTarget+Inlining+NGEN
    This PR (5042) - mean (689ms)  : 665, 712
     .   : milestone, 689,
    master - mean (689ms)  : 663, 715
     .   : milestone, 689,

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

    section CallTarget+Inlining+NGEN
    This PR (5042) - mean (1,143ms)  : 1122, 1163
     .   : milestone, 1143,
    master - mean (1,139ms)  : 1119, 1159
     .   : milestone, 1139,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5042) - mean (270ms)  : 265, 276
     .   : milestone, 270,
    master - mean (270ms)  : 266, 274
     .   : milestone, 270,

    section CallTarget+Inlining+NGEN
    This PR (5042) - mean (1,098ms)  : 1071, 1124
     .   : milestone, 1098,
    master - mean (1,102ms)  : 1080, 1124
     .   : milestone, 1102,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5042) - mean (262ms)  : 256, 267
     .   : milestone, 262,
    master - mean (261ms)  : 258, 263
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (5042) - mean (1,058ms)  : 1035, 1081
     .   : milestone, 1058,
    master - mean (1,064ms)  : 1034, 1094
     .   : milestone, 1064,

@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!

Loading
gantt
    title Throughput Linux x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5042) (11.082M)   : 0, 11081508
    master (10.866M)   : 0, 10866082
    benchmarks/2.9.0 (11.571M)   : 0, 11571320

    section Automatic
    This PR (5042) (7.537M)   : 0, 7536901
    master (7.540M)   : 0, 7540346
    benchmarks/2.9.0 (8.323M)   : 0, 8322791

    section Trace stats
    This PR (5042) (7.931M)   : 0, 7931206
    master (7.871M)   : 0, 7871047

    section Manual
    This PR (5042) (9.603M)   : 0, 9602531
    master (9.592M)   : 0, 9592317

    section Manual + Automatic
    This PR (5042) (7.190M)   : 0, 7190281
    master (7.111M)   : 0, 7111355

    section Version Conflict
    This PR (5042) (6.510M)   : 0, 6510398
    master (6.500M)   : 0, 6500033

Loading
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5042) (9.469M)   : 0, 9469010
    master (9.626M)   : 0, 9626140
    benchmarks/2.9.0 (9.660M)   : 0, 9659654

    section Automatic
    This PR (5042) (6.407M)   : 0, 6406625
    master (6.598M)   : 0, 6597508

    section Trace stats
    This PR (5042) (6.950M)   : 0, 6949973
    master (6.903M)   : 0, 6903022

    section Manual
    This PR (5042) (8.246M)   : 0, 8245857
    master (8.229M)   : 0, 8229482

    section Manual + Automatic
    This PR (5042) (6.298M)   : 0, 6298274
    master (5.942M)   : 0, 5941926

    section Version Conflict
    This PR (5042) (5.740M)   : 0, 5739759
    master (5.798M)   : 0, 5797646

Loading
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5042) (10.549M)   : 0, 10549039
    master (10.043M)   : 0, 10043350
    benchmarks/2.9.0 (9.963M)   : 0, 9963205

    section Automatic
    This PR (5042) (7.305M)   : 0, 7304932
    master (7.177M)   : 0, 7176524
    benchmarks/2.9.0 (7.483M)   : 0, 7483414

    section Trace stats
    This PR (5042) (7.642M)   : 0, 7641966
    master (7.524M)   : 0, 7523623

    section Manual
    This PR (5042) (9.129M)   : 0, 9129379
    master (8.868M)   : 0, 8868075

    section Manual + Automatic
    This PR (5042) (6.999M)   : 0, 6998721
    master (6.874M)   : 0, 6873629

    section Version Conflict
    This PR (5042) (6.332M)   : 0, 6332418
    master (6.235M)   : 0, 6235474

Loading
gantt
    title Throughput Linux x64 (ASM) (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    master (7.393M)   : 0, 7392821
    benchmarks/2.9.0 (7.887M)   : 0, 7887276

    section No attack
    master (1.755M)   : 0, 1754573
    benchmarks/2.9.0 (3.249M)   : 0, 3249034

    section Attack
    master (1.418M)   : 0, 1417862
    benchmarks/2.9.0 (2.565M)   : 0, 2565044

    section Blocking
    master (3.126M)   : 0, 3126330

    section IAST default
    master (6.441M)   : 0, 6440706

    section IAST full
    master (5.740M)   : 0, 5739720

    section Base vuln
    master (0.955M)   : 0, 955487

    section IAST vuln
    master (0.871M)   : 0, 870710

@lucaspimentel lucaspimentel changed the title [Tracer] Update signature parsing in the native tracer to account for ELEMENT_TYPE_PTR byte [Tracer] Update signature parsing in the tracer's native library to account for ELEMENT_TYPE_PTR byte Jan 16, 2024
@lucaspimentel lucaspimentel added type:bug area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) labels Jan 16, 2024
@zacharycmontoya zacharycmontoya merged commit 1b18a7a into master Jan 16, 2024
57 of 60 checks passed
@zacharycmontoya zacharycmontoya deleted the zach/profiler-sig-token-fix branch January 16, 2024 22:51
@github-actions github-actions bot added this to the vNext milestone Jan 16, 2024
@andrewlock andrewlock added the area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) label Jan 17, 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) type:bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants