[tracer] Add s3 instrumentation #6590

nhulston · 2025-01-23T20:45:09Z

Summary of changes

Creates an automatic instrumentation for S3's common & important operations (and their async equivalents):

Object management:

GetObject
PutObject
DeleteObject
CopyObject
ListObjectsV2

Bucket management:

PutBucket
DeleteBucket
ListBuckets

Multipart uploads:

InitiateMultipartUpload
UploadPart
CompleteMultipartUpload

Reason for change

More enriched spans for AWS SDK in S3
S3 is instrumented in other tracers
This is a prerequisite for a new feature I'm working on called 'span pointers'. See my PR in the Java tracer for more context.

Implementation details

Instrumented AmazonS3Client and the specified methods
Set bucket and object key tags (in alignment with other tracers) for operations where these fields exist

Test coverage

Unit tests:

cd tracer
dotnet test ./test/Datadog.Trace.ClrProfiler.Managed.Tests

Integration tests:

cd tracer

# Start docker localstock
docker run --rm -it -p 4566:4566 -p 4571:4571 -e SERVICES=s3 localstack/localstack

# Build tracer
./build.sh BuildTracerHome -buildConfiguration Debug -framework net6.0

# Run integation tests
./build.sh BuildAndRunOSxIntegrationTests -buildConfiguration Debug -framework net6.0 -Filter "Datadog.Trace.ClrProfiler.IntegrationTests.AWS.AwsS3Tests" -SampleName Samples.AWS.S3

Manually tested every operation (see above for images). All instrumentations work since each operation's span does have a custom resource name.

Other details

For AWS S3 v3.3 and v3.5, the bucket path is given as bucket-name/. For v3.7+, the bucket path is given as bucket-name. I had to manually handle this case in RuntimePipelineInvokeIntegration to get integration tests to pass and for the http.url tags to be consistent across versions. Let me know if there's a better way to handle this.

datadog-ddstaging · 2025-01-23T21:43:30Z

Datadog Report

Branch report: nicholas.hulston/add-s3-instrumentation
Commit report: 60aa1ba
Test service: dd-trace-dotnet

✅ 0 Failed, 565031 Passed, 5428 Skipped, 46h 34m 49.09s Total Time

andrewlock · 2025-01-23T22:17:10Z

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 (6590) - mean (75ms)  : 72, 78
     .   : milestone, 75,
    master - mean (75ms)  : 72, 78
     .   : milestone, 75,

    section CallTarget+Inlining+NGEN
    This PR (6590) - mean (1,033ms)  : 1008, 1057
     .   : milestone, 1033,
    master - mean (1,032ms)  : 1005, 1059
     .   : milestone, 1032,

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

    section CallTarget+Inlining+NGEN
    This PR (6590) - mean (712ms)  : 695, 728
     .   : milestone, 712,
    master - mean (711ms)  : 692, 731
     .   : milestone, 711,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6590) - mean (98ms)  : 97, 100
     .   : milestone, 98,
    master - mean (98ms)  : 96, 100
     .   : milestone, 98,

    section CallTarget+Inlining+NGEN
    This PR (6590) - mean (665ms)  : 649, 680
     .   : milestone, 665,
    master - mean (668ms)  : 656, 681
     .   : milestone, 668,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6590) - mean (190ms)  : 186, 195
     .   : milestone, 190,
    master - mean (191ms)  : 185, 196
     .   : milestone, 191,

    section CallTarget+Inlining+NGEN
    This PR (6590) - mean (1,100ms)  : 1068, 1132
     .   : milestone, 1100,
    master - mean (1,105ms)  : 1078, 1131
     .   : milestone, 1105,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6590) - mean (269ms)  : 265, 273
     .   : milestone, 269,
    master - mean (269ms)  : 264, 274
     .   : milestone, 269,

    section CallTarget+Inlining+NGEN
    This PR (6590) - mean (865ms)  : 830, 899
     .   : milestone, 865,
    master - mean (862ms)  : 835, 888
     .   : milestone, 862,

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

    section CallTarget+Inlining+NGEN
    This PR (6590) - mean (846ms)  : 815, 876
     .   : milestone, 846,
    master - mean (844ms)  : 808, 880
     .   : milestone, 844,

andrewlock · 2025-01-23T22:36:04Z

Benchmarks Report for tracer 🐌

Benchmarks for #6590 compared to master:

3 benchmarks are slower, with geometric mean 1.144
All benchmarks have the same 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.12μs	45.2ns	275ns	0.0195	0.00391	0	5.61 KB
master	`StartStopWithChild`	netcoreapp3.1	10.3μs	57.5ns	373ns	0.0204	0.0102	0	5.8 KB
master	`StartStopWithChild`	net472	16.6μs	66.7ns	258ns	1.06	0.32	0.107	6.21 KB
#6590	`StartStopWithChild`	net6.0	8μs	45.3ns	323ns	0.0156	0.00782	0	5.61 KB
#6590	`StartStopWithChild`	netcoreapp3.1	10.5μs	59.1ns	401ns	0.0208	0.00521	0	5.8 KB
#6590	`StartStopWithChild`	net472	16.6μs	43.9ns	170ns	1.06	0.329	0.101	6.21 KB

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	494μs	259ns	968ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	652μs	160ns	579ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	860μs	758ns	2.93μs	0.428	3.3 KB
#6590	`WriteAndFlushEnrichedTraces`	net6.0	471μs	368ns	1.42μs	0	2.7 KB
#6590	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	638μs	497ns	1.86μs	0	2.7 KB
#6590	`WriteAndFlushEnrichedTraces`	net472	848μs	687ns	2.66μs	0.425	3.3 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net6.0	128μs	293ns	1.14μs	0.192	14.47 KB
master	`SendRequest`	netcoreapp3.1	150μs	345ns	1.34μs	0.15	17.27 KB
master	`SendRequest`	net472	0.0034ns	0.000653ns	0.00253ns	0	0 b
#6590	`SendRequest`	net6.0	130μs	213ns	798ns	0.196	14.47 KB
#6590	`SendRequest`	netcoreapp3.1	148μs	282ns	1.09μs	0.223	17.27 KB
#6590	`SendRequest`	net472	0.000953ns	0.00038ns	0.00147ns	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	596μs	3.43μs	27μs	0.556	0	0	41.84 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	656μs	2.81μs	10.5μs	0.329	0	0	41.86 KB
master	`WriteAndFlushEnrichedTraces`	net472	839μs	4.2μs	17.8μs	8.87	2.82	0.403	53.3 KB
#6590	`WriteAndFlushEnrichedTraces`	net6.0	580μs	3.37μs	28.2μs	0.654	0	0	41.67 KB
#6590	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	692μs	3.95μs	36.2μs	0.321	0	0	41.78 KB
#6590	`WriteAndFlushEnrichedTraces`	net472	851μs	3.97μs	14.8μs	8.39	2.52	0.419	53.32 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net6.0	1.31μs	1.06ns	4.12ns	0.0144	0	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.75μs	1.68ns	6.49ns	0.014	0	1.02 KB
master	`ExecuteNonQuery`	net472	2.1μs	1.42ns	5.32ns	0.157	0.00105	987 B
#6590	`ExecuteNonQuery`	net6.0	1.37μs	1.12ns	4.33ns	0.0142	0	1.02 KB
#6590	`ExecuteNonQuery`	netcoreapp3.1	1.74μs	2.63ns	9.1ns	0.0132	0	1.02 KB
#6590	`ExecuteNonQuery`	net472	2.02μs	3.58ns	13.8ns	0.156	0.001	987 B

Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net6.0	1.29μs	0.805ns	3.12ns	0.0136	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.59μs	1.43ns	5.35ns	0.0136	976 B
master	`CallElasticsearch`	net472	2.57μs	1.26ns	4.88ns	0.158	995 B
master	`CallElasticsearchAsync`	net6.0	1.29μs	1.02ns	3.83ns	0.013	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.64μs	0.682ns	2.64ns	0.0138	1.02 KB
master	`CallElasticsearchAsync`	net472	2.76μs	1.71ns	6.62ns	0.167	1.05 KB
#6590	`CallElasticsearch`	net6.0	1.25μs	0.512ns	1.91ns	0.0139	976 B
#6590	`CallElasticsearch`	netcoreapp3.1	1.55μs	1.34ns	5.01ns	0.0133	976 B
#6590	`CallElasticsearch`	net472	2.56μs	1.19ns	4.6ns	0.157	995 B
#6590	`CallElasticsearchAsync`	net6.0	1.24μs	0.363ns	1.36ns	0.0136	952 B
#6590	`CallElasticsearchAsync`	netcoreapp3.1	1.6μs	1.61ns	6.24ns	0.0136	1.02 KB
#6590	`CallElasticsearchAsync`	net472	2.65μs	1.48ns	5.72ns	0.166	1.05 KB

Benchmarks.Trace.GraphQLBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6590

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑net6.0	1.124	1,302.22	1,463.92

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net6.0	1.3μs	0.536ns	2ns	0.0131	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.62μs	0.54ns	1.87ns	0.013	952 B
master	`ExecuteAsync`	net472	1.89μs	0.786ns	3.04ns	0.145	915 B
#6590	`ExecuteAsync`	net6.0	1.47μs	1.74ns	6.5ns	0.0133	952 B
#6590	`ExecuteAsync`	netcoreapp3.1	1.64μs	0.634ns	2.37ns	0.0123	952 B
#6590	`ExecuteAsync`	net472	1.82μs	0.629ns	2.43ns	0.144	915 B

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net6.0	4.41μs	2.29ns	8.88ns	0.0311	2.31 KB
master	`SendAsync`	netcoreapp3.1	5.3μs	2.42ns	9.37ns	0.0371	2.85 KB
master	`SendAsync`	net472	7.37μs	2.82ns	10.6ns	0.494	3.12 KB
#6590	`SendAsync`	net6.0	4.54μs	1.35ns	5.21ns	0.0317	2.31 KB
#6590	`SendAsync`	netcoreapp3.1	5.29μs	1.89ns	7.31ns	0.037	2.85 KB
#6590	`SendAsync`	net472	7.36μs	2.31ns	8.65ns	0.493	3.12 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	1.51μs	4.31ns	16.7ns	0.0233	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.27μs	1.4ns	5.4ns	0.0216	1.64 KB
master	`EnrichedLog`	net472	2.58μs	0.594ns	2.3ns	0.249	1.57 KB
#6590	`EnrichedLog`	net6.0	1.57μs	1.29ns	5.01ns	0.0228	1.64 KB
#6590	`EnrichedLog`	netcoreapp3.1	2.15μs	1.19ns	4.62ns	0.0227	1.64 KB
#6590	`EnrichedLog`	net472	2.53μs	1.46ns	5.65ns	0.249	1.57 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net6.0	111μs	132ns	512ns	0.0552	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	115μs	97.7ns	378ns	0.0575	0	4.28 KB
master	`EnrichedLog`	net472	150μs	190ns	737ns	0.672	0.224	4.46 KB
#6590	`EnrichedLog`	net6.0	113μs	204ns	792ns	0	0	4.28 KB
#6590	`EnrichedLog`	netcoreapp3.1	116μs	104ns	405ns	0.0579	0	4.28 KB
#6590	`EnrichedLog`	net472	150μs	148ns	574ns	0.672	0.224	4.46 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	3.05μs	1.32ns	5.12ns	0.0306	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.16μs	1.43ns	5.35ns	0.0291	2.2 KB
master	`EnrichedLog`	net472	4.89μs	1.31ns	5.06ns	0.32	2.02 KB
#6590	`EnrichedLog`	net6.0	3.04μs	1.02ns	3.94ns	0.0304	2.2 KB
#6590	`EnrichedLog`	netcoreapp3.1	4.16μs	6.3ns	24.4ns	0.0286	2.2 KB
#6590	`EnrichedLog`	net472	4.82μs	1.42ns	5.5ns	0.319	2.02 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net6.0	1.34μs	0.596ns	2.23ns	0.0161	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.84μs	0.791ns	2.96ns	0.0147	1.14 KB
master	`SendReceive`	net472	2.1μs	0.941ns	3.64ns	0.183	1.16 KB
#6590	`SendReceive`	net6.0	1.38μs	0.931ns	3.61ns	0.0158	1.14 KB
#6590	`SendReceive`	netcoreapp3.1	1.77μs	0.973ns	3.77ns	0.0149	1.14 KB
#6590	`SendReceive`	net472	2.08μs	0.845ns	3.16ns	0.183	1.16 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.85μs	0.984ns	3.68ns	0.0215	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	4.04μs	2.03ns	7.86ns	0.0221	1.65 KB
master	`EnrichedLog`	net472	4.35μs	2.5ns	9.67ns	0.322	2.04 KB
#6590	`EnrichedLog`	net6.0	2.77μs	1.45ns	5.62ns	0.0223	1.6 KB
#6590	`EnrichedLog`	netcoreapp3.1	3.95μs	1.1ns	4.1ns	0.0218	1.65 KB
#6590	`EnrichedLog`	net472	4.45μs	3.52ns	13.7ns	0.323	2.04 KB

Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6590

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0	1.156	413.09	477.42
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0	1.153	492.20	567.52

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	413ns	0.196ns	0.76ns	0.00816	576 B
master	`StartFinishSpan`	netcoreapp3.1	573ns	0.483ns	1.87ns	0.00776	576 B
master	`StartFinishSpan`	net472	680ns	0.305ns	1.18ns	0.0917	578 B
master	`StartFinishScope`	net6.0	492ns	0.221ns	0.856ns	0.00983	696 B
master	`StartFinishScope`	netcoreapp3.1	690ns	0.303ns	1.05ns	0.00936	696 B
master	`StartFinishScope`	net472	877ns	0.786ns	3.04ns	0.104	658 B
#6590	`StartFinishSpan`	net6.0	477ns	0.41ns	1.59ns	0.00809	576 B
#6590	`StartFinishSpan`	netcoreapp3.1	614ns	1.19ns	4.59ns	0.00757	576 B
#6590	`StartFinishSpan`	net472	647ns	0.214ns	0.83ns	0.0918	578 B
#6590	`StartFinishScope`	net6.0	568ns	0.569ns	2.21ns	0.00986	696 B
#6590	`StartFinishScope`	netcoreapp3.1	709ns	0.429ns	1.66ns	0.00961	696 B
#6590	`StartFinishScope`	net472	878ns	0.3ns	1.12ns	0.104	658 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	700ns	0.288ns	1.12ns	0.00981	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	1μs	0.552ns	2.07ns	0.00907	696 B
master	`RunOnMethodBegin`	net472	1.11μs	0.735ns	2.75ns	0.104	658 B
#6590	`RunOnMethodBegin`	net6.0	644ns	0.285ns	1.1ns	0.00967	696 B
#6590	`RunOnMethodBegin`	netcoreapp3.1	997ns	0.483ns	1.81ns	0.00954	696 B
#6590	`RunOnMethodBegin`	net472	1.1μs	0.884ns	3.42ns	0.104	658 B

… and object key

…eMultipartUpload, and CompleteMultipartUploadAsync

# Conflicts: # tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/TelemetryMetricGenerator/CiVisibilityMetricsTelemetryCollector_CountCIVisibility.g.cs # tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/TelemetryMetricGenerator/CiVisibilityMetricsTelemetryCollector_DistributionCIVisibility.g.cs # tracer/src/Datadog.Trace/Generated/net6.0/Datadog.Trace.SourceGenerators/TelemetryMetricGenerator/CiVisibilityMetricsTelemetryCollector_CountCIVisibility.g.cs # tracer/src/Datadog.Trace/Generated/net6.0/Datadog.Trace.SourceGenerators/TelemetryMetricGenerator/CiVisibilityMetricsTelemetryCollector_DistributionCIVisibility.g.cs # tracer/src/Datadog.Trace/Generated/netcoreapp3.1/Datadog.Trace.SourceGenerators/TelemetryMetricGenerator/CiVisibilityMetricsTelemetryCollector_CountCIVisibility.g.cs # tracer/src/Datadog.Trace/Generated/netcoreapp3.1/Datadog.Trace.SourceGenerators/TelemetryMetricGenerator/CiVisibilityMetricsTelemetryCollector_DistributionCIVisibility.g.cs # tracer/src/Datadog.Trace/Generated/netstandard2.0/Datadog.Trace.SourceGenerators/TelemetryMetricGenerator/CiVisibilityMetricsTelemetryCollector_CountCIVisibility.g.cs # tracer/src/Datadog.Trace/Generated/netstandard2.0/Datadog.Trace.SourceGenerators/TelemetryMetricGenerator/CiVisibilityMetricsTelemetryCollector_DistributionCIVisibility.g.cs # tracer/src/Datadog.Tracer.Native/Generated/generated_calltargets.g.cpp

…on't have tags for bucket or object key

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AWS/S3/AwsS3Common.cs

bouwkast

Still reviewing, added some comments, nothing big

We'll need some .NET Framework snapshots, I don't see them in the artifacts, so I'll just plan on generating them locally tomorrow.

You'll need to rebase on master or merge master into your branch to fix the failing nullability file check: https://github.com/DataDog/dd-trace-dotnet/actions/runs/13123066841/job/36613352456?pr=6590

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AWS/S3/AwsS3Common.cs

bouwkast · 2025-02-04T21:57:18Z

tracer/test/test-applications/integrations/Samples.AWS.S3/Program.cs

+            // Fixes localstack breaking change in AWSSSDK.S3 versions 3.7.412.0 and above.
+            // https://github.com/aws/aws-sdk-net/issues/3610


Thanks for the callout!

bouwkast

Looks good 🙇

I have a few requests on adding some additional tests as a couple of the instrumentations were missed (from what I can see in the sample)

And then just some span count stuff in the tests

Feel free to reach out for running the .NET Framework ones!

bouwkast · 2025-02-05T19:13:32Z

...tadog.Trace/ClrProfiler/AutoInstrumentation/AWS/SDK/RuntimePipelineInvokeAsyncIntegration.cs

+                    if (tags is AwsS3Tags s3Tags)
                    {
-                        null => absolutePath,
-                        string resourcePath when absolutePath == "/" => resourcePath,
-                        string resourcePath => UriHelpers.Combine(absolutePath, resourcePath),
-                    };
+                        var path = AwsS3Common.BuildS3Path(s3Tags);
+                        tags.HttpUrl = $"{uri?.Scheme}{Uri.SchemeDelimiter}{uri?.Authority}{path}";
+                    }
+                    else
+                    {
+                        // Fallback to the "generic" approach for other AWS services
+                        var absolutePath = uri?.AbsolutePath;
+                        var path = request.ResourcePath switch
+                        {
+                            null => absolutePath,
+                            string resourcePath when absolutePath == "/" => resourcePath,
+                            string resourcePath => UriHelpers.Combine(absolutePath, resourcePath),
+                        };
+                        tags.HttpUrl = $"{uri?.Scheme}{Uri.SchemeDelimiter}{uri?.Authority}{path}";
+                    }


This appears to be copied to RuntimePipelineInvokeSyncIntegration could be a good spot for extracting out this logic somewhere?

But fine to leave as is as I wouldn't think these would need to change much

Level of care is low for this by the way.

I won't need the changes in these two files after implementing the scrubber; will revert these two files

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AWS/S3/AwsS3Common.cs

bouwkast · 2025-02-05T20:11:21Z

tracer/test/test-applications/integrations/Samples.AWS.S3/AsyncHelpers.cs

Cross referencing the instrumentations I'm not seeing calls for DeleteObjectsAsync and GetObjectAsync

bouwkast · 2025-02-05T20:12:29Z

tracer/test/test-applications/integrations/Samples.AWS.S3/SyncHelpers.cs

Cross referencing the instrumentations I'm not seeing calls for DeleteObjects and GetObject

When added I can re-generate the snapshots since these are .NET Framework (unless they run on CI after being added)

bouwkast · 2025-02-05T20:14:06Z

tracer/test/test-applications/integrations/Samples.AWS.S3/SyncHelpers.cs

@@ -0,0 +1,224 @@
+#if NETFRAMEWORK


Just want to verify that the .NET / .NET Core versions do not have access to these sync methods?

Yes, e.g. with my manual testing on Lambda, I could only call the async methods but not the sync versions

bouwkast · 2025-02-05T20:17:02Z

tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/AWS/AwsS3Tests.cs

+                var expectedCount = 4;
+                var frameworkName = "NetCore";
+#endif
+                var spans = agent.WaitForSpans(expectedCount);


nitpicky, but two things:

expectedCount is incorrect, there are definitely more spans that we are expecting (and getting) could both .NET Core and .NET Framework get updated? I expect these would also change when adding in the additional methods for the missed instrumentations.

Additionally (not as important IMO), could you add a check for spans.Count().Should().Be(expectedCount)?

…om url tag builder

nhulston added 5 commits January 27, 2025 17:03

add basic S3 instrumentation for PutObject and PutObjectAsync

abb2bae

implement tags

99d4528

create basic s3 sample app

1c6707a

implement basic s3 integration tests

01734d3

run GeneratePackageVersions

8e22164

nhulston force-pushed the nicholas.hulston/add-s3-instrumentation branch from baa2cad to 8e22164 Compare January 27, 2025 22:24

nhulston added 4 commits January 28, 2025 11:30

Add aws.bucket.name and aws.object.key tags

1e3ee72

Instrument PutObject and PutObjectAsync; set tags for bucket name…

96dde22

… and object key

create basic instrumentation for CopyObject, CopyObjectAsync, Complet…

f1cde9c

…eMultipartUpload, and CompleteMultipartUploadAsync

Run GeneratePackageVersions to update call targets

2085dfb

nhulston force-pushed the nicholas.hulston/add-s3-instrumentation branch from cdea333 to 2085dfb Compare January 28, 2025 17:32

nhulston added 16 commits January 28, 2025 13:36

update calltargets

205388b

Instrument CopyObject and CompleteMultipartUpload

308d567

create GetObject integration

387122d

create DeleteObject integration

4bffcbc

create ListObjects integration

c9eb8e2

create PutBucket integration

0e4c0f0

create DeleteBucket integration

a6d606f

refactor async integrations

239409f

create ListObjectsV2 integration

9273310

refactor integrations; remove ListObjectsV1 integration in favor of v2

5e415a7

run GeneratePackageVersions

675d9e9

create ListBuckets integration

57d7e35

simplify comments

dca6ed4

create DeleteObjects integration

ed525ff

create InitiateMultipartUpload integration

6493d30

nhulston force-pushed the nicholas.hulston/add-s3-instrumentation branch from a5d213e to 32b8589 Compare January 30, 2025 20:26

Merge branch 'master' into nicholas.hulston/add-s3-instrumentation

4163a58

nhulston force-pushed the nicholas.hulston/add-s3-instrumentation branch from d603384 to 4163a58 Compare January 31, 2025 14:40

DataDog deleted a comment from github-actions bot Jan 31, 2025

nhulston and others added 5 commits January 31, 2025 09:41

Merge branch 'master' into nicholas.hulston/add-s3-instrumentation

65c52bd

update snapshots

7d631d0

manually construct s3 request paths

f3cb287

add test coverage for every instrumented operation

b2b2f2d

update rules and snapshots. Operations like ListBuckets obviously w…

dc83a86

…on't have tags for bucket or object key

nhulston force-pushed the nicholas.hulston/add-s3-instrumentation branch from e42f14c to e7edd48 Compare February 3, 2025 18:16

nhulston requested a review from DylanLovesCoffee February 3, 2025 18:45

fix localstack checksum error in ci. aws/aws-sdk-net#3610

6fca491

nhulston force-pushed the nicholas.hulston/add-s3-instrumentation branch from e7edd48 to 6fca491 Compare February 3, 2025 20:06

Merge branch 'master' into nicholas.hulston/add-s3-instrumentation

340942b

nhulston commented Feb 4, 2025

View reviewed changes

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AWS/S3/AwsS3Common.cs Outdated Show resolved Hide resolved

bouwkast reviewed Feb 4, 2025

View reviewed changes

bouwkast and others added 2 commits February 5, 2025 09:03

Add .NET Framework snapshots

7e6ad26

Merge branch 'master' into nicholas.hulston/add-s3-instrumentation

98ddff9

nhulston requested a review from bouwkast February 5, 2025 20:05

bouwkast reviewed Feb 5, 2025

View reviewed changes

nhulston added 5 commits February 5, 2025 16:31

Add regex scrubber for http.url tag in integration tests; revert cust…

02e2bad

…om url tag builder

revert RUntimePipelineInvokeIntegration files

d14d9c0

Add DeleteObjectsAsync and GetObjectAsync to integration tests

621491c

Update NetCore snapshots

2dace21

Test expected span count

c7f486a

nhulston requested a review from bouwkast February 5, 2025 21:51

DylanLovesCoffee approved these changes Feb 6, 2025

View reviewed changes

bouwkast and others added 3 commits February 6, 2025 11:28

Update .NET Framework snapshots

00b8041

Merge branch 'master' into nicholas.hulston/add-s3-instrumentation

3a2d0f8

run GeneratePackageVersions

60aa1ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tracer] Add s3 instrumentation #6590

[tracer] Add s3 instrumentation #6590

nhulston commented Jan 23, 2025 •

edited

Loading

datadog-ddstaging bot commented Jan 23, 2025 •

edited

Loading

andrewlock commented Jan 23, 2025 •

edited

Loading

andrewlock commented Jan 23, 2025 •

edited

Loading

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6590

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6590

Raw results

Raw results

bouwkast left a comment

bouwkast Feb 4, 2025

bouwkast left a comment

bouwkast Feb 5, 2025

bouwkast Feb 5, 2025

nhulston Feb 5, 2025

bouwkast Feb 5, 2025

bouwkast Feb 5, 2025

bouwkast Feb 5, 2025

nhulston Feb 5, 2025

bouwkast Feb 5, 2025

		// Fixes localstack breaking change in AWSSSDK.S3 versions 3.7.412.0 and above.
		// https://github.com/aws/aws-sdk-net/issues/3610

[tracer] Add s3 instrumentation #6590

Are you sure you want to change the base?

[tracer] Add s3 instrumentation #6590

Conversation

nhulston commented Jan 23, 2025 • edited Loading

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

datadog-ddstaging bot commented Jan 23, 2025 • edited Loading

Datadog Report

andrewlock commented Jan 23, 2025 • edited Loading

Execution-Time Benchmarks Report ⏱️

andrewlock commented Jan 23, 2025 • edited Loading

Benchmarks Report for tracer 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6590

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6590

Raw results

Raw results

bouwkast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bouwkast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhulston commented Jan 23, 2025 •

edited

Loading

datadog-ddstaging bot commented Jan 23, 2025 •

edited

Loading

andrewlock commented Jan 23, 2025 •

edited

Loading

andrewlock commented Jan 23, 2025 •

edited

Loading