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

Add db name & host to sql injected tags #5278

Merged
merged 3 commits into from
Mar 8, 2024
Merged

Add db name & host to sql injected tags #5278

merged 3 commits into from
Mar 8, 2024

Conversation

vandonr
Copy link
Contributor

@vandonr vandonr commented Mar 6, 2024 •
edited
Loading

Summary of changes

Adding some tag values that were already present on the span as sql comments

Reason for change

https://datadoghq.atlassian.net/browse/AIT-9565

Implementation details

Test coverage

✅

Other details

same feature in other tracers:
DataDog/dd-trace-go#2550
DataDog/dd-trace-py#8286
DataDog/dd-trace-java#6778

@vandonr vandonr requested a review from a team as a code owner March 6, 2024 14:01
@vandonr vandonr marked this pull request as draft March 6, 2024 14:02
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Mar 6, 2024 •
edited
Loading

Datadog Report

Branch report: vandonr/wip
Commit report: df25781
Test service: dd-trace-dotnet

✅ 0 Failed, 345998 Passed, 1584 Skipped, 57m 22.2s Wall Time
❄️ 1 New Flaky

New Flaky Tests (1)

  • Async1Test - Datadog.Trace.Tests.Util.Delegates.FuncInstrumentationTests - Last Failure

    Expand for error 
     Expected value to be 43, but found 0.

@andrewlock
Copy link
Member

andrewlock commented Mar 6, 2024 •
edited
Loading

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 (5278) - mean (73ms)  : 65, 82
     .   : milestone, 73,
    master - mean (74ms)  : 66, 83
     .   : milestone, 74,

    section CallTarget+Inlining+NGEN
    This PR (5278) - mean (989ms)  : 960, 1018
     .   : milestone, 989,
    master - mean (996ms)  : 972, 1020
     .   : milestone, 996,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5278) - mean (111ms)  : 108, 114
     .   : milestone, 111,
    master - mean (111ms)  : 108, 115
     .   : milestone, 111,

    section CallTarget+Inlining+NGEN
    This PR (5278) - mean (711ms)  : 690, 732
     .   : milestone, 711,
    master - mean (717ms)  : 686, 749
     .   : milestone, 717,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5278) - mean (94ms)  : 91, 97
     .   : milestone, 94,
    master - mean (94ms)  : 91, 97
     .   : milestone, 94,

    section CallTarget+Inlining+NGEN
    This PR (5278) - mean (670ms)  : 645, 695
     .   : milestone, 670,
    master - mean (676ms)  : 641, 712
     .   : milestone, 676,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5278) - mean (187ms)  : 184, 191
     .   : milestone, 187,
    master - mean (188ms)  : 186, 190
     .   : milestone, 188,

    section CallTarget+Inlining+NGEN
    This PR (5278) - mean (1,062ms)  : 1041, 1082
     .   : milestone, 1062,
    master - mean (1,064ms)  : 1037, 1091
     .   : milestone, 1064,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5278) - mean (271ms)  : 266, 275
     .   : milestone, 271,
    master - mean (271ms)  : 268, 275
     .   : milestone, 271,

    section CallTarget+Inlining+NGEN
    This PR (5278) - mean (871ms)  : 843, 898
     .   : milestone, 871,
    master - mean (871ms)  : 843, 899
     .   : milestone, 871,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5278) - mean (260ms)  : 256, 263
     .   : milestone, 260,
    master - mean (261ms)  : 257, 265
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (5278) - mean (847ms)  : 817, 876
     .   : milestone, 847,
    master - mean (857ms)  : 833, 881
     .   : milestone, 857,
Loading

@vandonr vandonr changed the title add db name to sql injected tags Add db name & host to sql injected tags Mar 6, 2024
@vandonr vandonr marked this pull request as ready for review March 7, 2024 09:23
@vandonr vandonr requested a review from a team as a code owner March 7, 2024 09:23
@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 (5278) (11.208M)   : 0, 11208038
    master (11.007M)   : 0, 11006674
    benchmarks/2.9.0 (11.269M)   : 0, 11269489

    section Automatic
    This PR (5278) (7.913M)   : 0, 7912589
    master (7.556M)   : 0, 7555579
    benchmarks/2.9.0 (8.237M)   : 0, 8236748

    section Trace stats
    This PR (5278) (8.053M)   : 0, 8052736
    master (7.855M)   : 0, 7855147

    section Manual
    This PR (5278) (9.791M)   : 0, 9790718
    master (9.556M)   : 0, 9555551

    section Manual + Automatic
    This PR (5278) (7.217M)   : 0, 7217239
    master (7.141M)   : 0, 7140945

    section Version Conflict
    This PR (5278) (6.438M)   : 0, 6437932
    master (6.452M)   : 0, 6451971
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5278) (9.627M)   : 0, 9627087
    master (9.496M)   : 0, 9495623
    benchmarks/2.9.0 (9.335M)   : 0, 9334561

    section Automatic
    This PR (5278) (6.734M)   : 0, 6733822
    master (6.550M)   : 0, 6550088

    section Trace stats
    This PR (5278) (6.997M)   : 0, 6996954
    master (6.930M)   : 0, 6929825

    section Manual
    This PR (5278) (8.482M)   : 0, 8482346
    master (8.294M)   : 0, 8293659

    section Manual + Automatic
    This PR (5278) (6.172M)   : 0, 6172096
    master (6.274M)   : 0, 6273873

    section Version Conflict
    This PR (5278) (5.838M)   : 0, 5838348
    master (5.748M)   : 0, 5748183
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5278) (10.287M)   : 0, 10286531
    master (10.120M)   : 0, 10120352
    benchmarks/2.9.0 (10.334M)   : 0, 10333846

    section Automatic
    This PR (5278) (7.463M)   : 0, 7463424
    master (7.343M)   : 0, 7343410
    benchmarks/2.9.0 (7.377M)   : 0, 7377326

    section Trace stats
    This PR (5278) (7.702M)   : 0, 7702196
    master (7.630M)   : 0, 7629966

    section Manual
    This PR (5278) (9.190M)   : 0, 9189755
    master (8.884M)   : 0, 8883719

    section Manual + Automatic
    This PR (5278) (7.093M)   : 0, 7092655
    master (6.989M)   : 0, 6989119

    section Version Conflict
    This PR (5278) (6.426M)   : 0, 6425725
    master (6.322M)   : 0, 6322239
Loading 
gantt
    title Throughput Linux x64 (ASM) (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    master (7.454M)   : 0, 7454237
    benchmarks/2.9.0 (7.886M)   : 0, 7885996

    section No attack
    master (1.841M)   : 0, 1841297
    benchmarks/2.9.0 (3.256M)   : 0, 3255652

    section Attack
    master (1.454M)   : 0, 1454025
    benchmarks/2.9.0 (2.490M)   : 0, 2489577

    section Blocking
    master (3.195M)   : 0, 3194705

    section IAST default
    master (6.463M)   : 0, 6462928

    section IAST full
    master (5.595M)   : 0, 5594918

    section Base vuln
    master (0.946M)   : 0, 945501

    section IAST vuln
    master (0.849M)   : 0, 849226
Loading

andrewlock
andrewlock approved these changes Mar 7, 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.

Nice, LGTM 👍

@vandonr vandonr merged commit 31fa50b into master Mar 8, 2024
56 of 58 checks passed
@vandonr vandonr deleted the vandonr/wip branch March 8, 2024 12:21
@github-actions github-actions bot added this to the vNext milestone Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewlock andrewlock andrewlock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.49.0
Development

Successfully merging this pull request may close these issues.
2 participants
@vandonr @andrewlock