Add Google Protobuf Instrumentation #6166

vandonr · 2024-10-18T14:58:36Z

Summary of changes

Adds a new instrumentation for Google Protobuf (not https://github.com/protobuf-net/protobuf-net)
The instrumentation doesn't create any new spans, we are just adding tags on existing ones. The main one being a tag containing the full protobuf schema as an OpenAPI json. Since extracting the schema is costly, we only do it once every 30 seconds (hardcoded for now).

Reason for change

On request from DSM team, as an equivalent of its java counterpart: https://github.com/DataDog/dd-trace-java/tree/master/dd-java-agent/instrumentation/protobuf

Implementation details

There is a cache <schema name -> schema json>, but I wanted to control the size, so I ended up repurposing the code that had been written for DbConnectionCache, where it's using a cache for small cardinality, but if there are too many different values, it stops caching.
I wrote a rate limiter rather than reusing one because to avoid forcing the sampling decision on every span, I needed one where I can peek the rate limit decision without "consuming" it.

Test coverage

Added a sample app that serializes and deserializes a protobuf message. Spans are created manually since this integration doesn't create one.

Other details

datadog-ddstaging · 2024-10-18T15:18:02Z

Datadog Report

Branch report: vandonr/proto
Commit report: 3236d38
Test service: dd-trace-dotnet

✅ 0 Failed, 558477 Passed, 4621 Skipped, 46h 31m 20.16s Total Time

andrewlock · 2024-10-18T15:48:22Z

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 (6166) - mean (69ms)  : 66, 73
     .   : milestone, 69,
    master - mean (69ms)  : 67, 71
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (6166) - mean (993ms)  : 969, 1018
     .   : milestone, 993,
    master - mean (987ms)  : 967, 1008
     .   : milestone, 987,

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

    section CallTarget+Inlining+NGEN
    This PR (6166) - mean (672ms)  : 655, 688
     .   : milestone, 672,
    master - mean (670ms)  : 652, 688
     .   : milestone, 670,

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

    section CallTarget+Inlining+NGEN
    This PR (6166) - mean (626ms)  : 609, 643
     .   : milestone, 626,
    master - mean (633ms)  : 610, 656
     .   : milestone, 633,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6166) - mean (190ms)  : 186, 194
     .   : milestone, 190,
    master - mean (190ms)  : 186, 193
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (6166) - mean (1,099ms)  : 1069, 1129
     .   : milestone, 1099,
    master - mean (1,094ms)  : 1071, 1117
     .   : milestone, 1094,

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

    section CallTarget+Inlining+NGEN
    This PR (6166) - mean (865ms)  : 827, 903
     .   : milestone, 865,
    master - mean (861ms)  : 829, 893
     .   : milestone, 861,

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

    section CallTarget+Inlining+NGEN
    This PR (6166) - mean (841ms)  : 807, 876
     .   : milestone, 841,
    master - mean (841ms)  : 810, 872
     .   : milestone, 841,

andrewlock · 2024-10-18T16:14:41Z

Throughput/Crank Report ⚡

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 (6166) (11.162M)   : 0, 11161793
    master (11.223M)   : 0, 11223304
    benchmarks/2.9.0 (11.045M)   : 0, 11045405

    section Automatic
    This PR (6166) (7.265M)   : 0, 7264852
    master (7.369M)   : 0, 7368879
    benchmarks/2.9.0 (7.885M)   : 0, 7885346

    section Trace stats
    master (7.636M)   : 0, 7635764

    section Manual
    master (11.208M)   : 0, 11208283

    section Manual + Automatic
    This PR (6166) (6.810M)   : 0, 6810342
    master (6.860M)   : 0, 6860273

    section DD_TRACE_ENABLED=0
    master (10.386M)   : 0, 10385517

gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6166) (9.764M)   : 0, 9764307
    master (9.546M)   : 0, 9546423
    benchmarks/2.9.0 (9.586M)   : 0, 9586476

    section Automatic
    This PR (6166) (6.449M)   : 0, 6448736
    master (6.583M)   : 0, 6583133

    section Trace stats
    master (6.861M)   : 0, 6860986

    section Manual
    master (9.767M)   : 0, 9766866

    section Manual + Automatic
    This PR (6166) (5.942M)   : 0, 5941979
    master (5.972M)   : 0, 5972206

    section DD_TRACE_ENABLED=0
    master (8.948M)   : 0, 8947586

gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6166) (9.861M)   : 0, 9861124

    section Automatic
    This PR (6166) (6.395M)   : 0, 6394947

    section Manual + Automatic
    This PR (6166) (5.984M)   : 0, 5983994

andrewlock · 2024-10-18T16:15:39Z

Benchmarks Report for tracer 🐌

Benchmarks for #6166 compared to master:

1 benchmarks are faster, with geometric mean 1.133
2 benchmarks are slower, with geometric mean 1.223
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	7.89μs	44.2ns	280ns	0.0151	0.00753	0	5.61 KB
master	`StartStopWithChild`	netcoreapp3.1	9.86μs	47.4ns	183ns	0.0147	0.0049	0	5.8 KB
master	`StartStopWithChild`	net472	15.9μs	49.3ns	191ns	1.03	0.293	0.0951	6.2 KB
#6166	`StartStopWithChild`	net6.0	7.97μs	45.7ns	342ns	0.0118	0.00393	0	5.61 KB
#6166	`StartStopWithChild`	netcoreapp3.1	10.1μs	55.9ns	326ns	0.0352	0.0151	0.00503	5.8 KB
#6166	`StartStopWithChild`	net472	16.3μs	32.6ns	122ns	1.04	0.312	0.0961	6.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	485μs	459ns	1.78μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	647μs	199ns	717ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	864μs	720ns	2.69μs	0.431	3.3 KB
#6166	`WriteAndFlushEnrichedTraces`	net6.0	486μs	212ns	794ns	0	2.7 KB
#6166	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	694μs	276ns	1.03μs	0	2.7 KB
#6166	`WriteAndFlushEnrichedTraces`	net472	853μs	496ns	1.92μ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	130μs	412ns	1.49μs	0.198	14.47 KB
master	`SendRequest`	netcoreapp3.1	151μs	317ns	1.23μs	0.227	17.27 KB
master	`SendRequest`	net472	0.00528ns	0.00135ns	0.00523ns	0	0 b
#6166	`SendRequest`	net6.0	131μs	531ns	2.06μs	0.197	14.47 KB
#6166	`SendRequest`	netcoreapp3.1	144μs	289ns	1.12μs	0.22	17.27 KB
#6166	`SendRequest`	net472	0.00356ns	0.00113ns	0.00436ns	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	560μs	3.02μs	16μs	0.543	0	0	41.4 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	653μs	3.6μs	22.2μs	0.324	0	0	41.65 KB
master	`WriteAndFlushEnrichedTraces`	net472	833μs	2.96μs	11.5μs	8.12	2.44	0.406	53.3 KB
#6166	`WriteAndFlushEnrichedTraces`	net6.0	567μs	2.99μs	16.9μs	0.551	0	0	41.6 KB
#6166	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	634μs	2.13μs	7.66μs	0.322	0	0	41.63 KB
#6166	`WriteAndFlushEnrichedTraces`	net472	837μs	3.42μs	13.2μs	8.45	2.53	0.422	53.33 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.28μs	0.982ns	3.67ns	0.0141	0	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.79μs	1.76ns	6.82ns	0.0134	0	1.02 KB
master	`ExecuteNonQuery`	net472	2.03μs	2.01ns	7.51ns	0.156	0.00101	987 B
#6166	`ExecuteNonQuery`	net6.0	1.31μs	0.492ns	1.84ns	0.0139	0	1.02 KB
#6166	`ExecuteNonQuery`	netcoreapp3.1	1.72μs	1.3ns	4.87ns	0.0138	0	1.02 KB
#6166	`ExecuteNonQuery`	net472	1.98μs	1.63ns	6.33ns	0.156	0.000987	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.23μs	0.976ns	3.78ns	0.0136	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.56μs	1.29ns	4.83ns	0.0134	976 B
master	`CallElasticsearch`	net472	2.56μs	1.97ns	7.64ns	0.157	995 B
master	`CallElasticsearchAsync`	net6.0	1.26μs	1.11ns	4.14ns	0.0132	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.67μs	0.959ns	3.71ns	0.0134	1.02 KB
master	`CallElasticsearchAsync`	net472	2.52μs	1.31ns	5.08ns	0.166	1.05 KB
#6166	`CallElasticsearch`	net6.0	1.13μs	0.585ns	2.19ns	0.0134	976 B
#6166	`CallElasticsearch`	netcoreapp3.1	1.59μs	0.677ns	2.53ns	0.0127	976 B
#6166	`CallElasticsearch`	net472	2.61μs	1.04ns	3.91ns	0.158	995 B
#6166	`CallElasticsearchAsync`	net6.0	1.24μs	0.922ns	3.33ns	0.0136	952 B
#6166	`CallElasticsearchAsync`	netcoreapp3.1	1.66μs	0.84ns	3.25ns	0.014	1.02 KB
#6166	`CallElasticsearchAsync`	net472	2.57μs	1.89ns	7.06ns	0.166	1.05 KB

Benchmarks.Trace.GraphQLBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6166

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑net6.0	1.133	1,443.30	1,274.00

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net6.0	1.44μs	0.83ns	3.21ns	0.0137	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.63μs	3.62ns	14ns	0.0122	952 B
master	`ExecuteAsync`	net472	1.77μs	0.313ns	1.17ns	0.145	915 B
#6166	`ExecuteAsync`	net6.0	1.27μs	1.69ns	6.56ns	0.0135	952 B
#6166	`ExecuteAsync`	netcoreapp3.1	1.62μs	4.56ns	17.7ns	0.0129	952 B
#6166	`ExecuteAsync`	net472	1.81μs	0.64ns	2.48ns	0.145	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.32μs	1ns	3.74ns	0.0324	2.31 KB
master	`SendAsync`	netcoreapp3.1	5.23μs	1.88ns	7.3ns	0.0393	2.85 KB
master	`SendAsync`	net472	7.38μs	2.24ns	8.68ns	0.495	3.12 KB
#6166	`SendAsync`	net6.0	4.4μs	2.03ns	7.6ns	0.0308	2.31 KB
#6166	`SendAsync`	netcoreapp3.1	5.18μs	1.87ns	7.24ns	0.0391	2.85 KB
#6166	`SendAsync`	net472	7.44μs	2.55ns	9.55ns	0.492	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.45μs	0.721ns	2.7ns	0.0228	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.14μs	1.27ns	4.74ns	0.0222	1.64 KB
master	`EnrichedLog`	net472	2.52μs	0.772ns	2.89ns	0.249	1.57 KB
#6166	`EnrichedLog`	net6.0	1.49μs	0.58ns	2.17ns	0.0231	1.64 KB
#6166	`EnrichedLog`	netcoreapp3.1	2.28μs	0.951ns	3.68ns	0.0216	1.64 KB
#6166	`EnrichedLog`	net472	2.58μs	1.03ns	3.98ns	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	112μs	123ns	475ns	0.0561	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	116μs	144ns	537ns	0	0	4.28 KB
master	`EnrichedLog`	net472	151μs	224ns	868ns	0.672	0.224	4.46 KB
#6166	`EnrichedLog`	net6.0	112μs	163ns	631ns	0.0563	0	4.28 KB
#6166	`EnrichedLog`	netcoreapp3.1	117μs	156ns	606ns	0.0586	0	4.28 KB
#6166	`EnrichedLog`	net472	150μs	201ns	780ns	0.674	0.225	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	2.95μs	1.38ns	5.36ns	0.0311	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.13μs	1.53ns	5.72ns	0.029	2.2 KB
master	`EnrichedLog`	net472	4.94μs	1.29ns	5ns	0.321	2.02 KB
#6166	`EnrichedLog`	net6.0	3.05μs	0.886ns	3.31ns	0.0305	2.2 KB
#6166	`EnrichedLog`	netcoreapp3.1	4.24μs	1.19ns	4.6ns	0.0297	2.2 KB
#6166	`EnrichedLog`	net472	4.93μs	1.95ns	7.29ns	0.32	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.41μs	1.49ns	5.79ns	0.0157	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.76μs	0.847ns	3.28ns	0.015	1.14 KB
master	`SendReceive`	net472	2.12μs	7.29ns	28.2ns	0.183	1.16 KB
#6166	`SendReceive`	net6.0	1.32μs	1.11ns	4.16ns	0.0159	1.14 KB
#6166	`SendReceive`	netcoreapp3.1	1.78μs	0.438ns	1.64ns	0.0152	1.14 KB
#6166	`SendReceive`	net472	2.11μs	1.32ns	5.1ns	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.73μs	1.03ns	3.85ns	0.0218	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.79μs	1.75ns	6.79ns	0.0227	1.65 KB
master	`EnrichedLog`	net472	4.44μs	2.6ns	9.73ns	0.322	2.04 KB
#6166	`EnrichedLog`	net6.0	2.67μs	5.49ns	21.3ns	0.0216	1.6 KB
#6166	`EnrichedLog`	netcoreapp3.1	3.8μs	1.89ns	7.33ns	0.021	1.65 KB
#6166	`EnrichedLog`	net472	4.49μs	5.56ns	21.5ns	0.323	2.04 KB

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

Slower ⚠️ in #6166

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1	1.233	541.36	667.46
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0	1.213	386.53	469.05

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	387ns	0.469ns	1.82ns	0.00807	576 B
master	`StartFinishSpan`	netcoreapp3.1	542ns	1.1ns	4.24ns	0.00782	576 B
master	`StartFinishSpan`	net472	597ns	1.37ns	5.31ns	0.0917	578 B
master	`StartFinishScope`	net6.0	601ns	0.998ns	3.87ns	0.00983	696 B
master	`StartFinishScope`	netcoreapp3.1	760ns	1.81ns	6.99ns	0.00935	696 B
master	`StartFinishScope`	net472	843ns	2.07ns	8ns	0.104	658 B
#6166	`StartFinishSpan`	net6.0	469ns	0.519ns	2.01ns	0.00802	576 B
#6166	`StartFinishSpan`	netcoreapp3.1	668ns	0.877ns	3.4ns	0.00762	576 B
#6166	`StartFinishSpan`	net472	578ns	1.28ns	4.95ns	0.0917	578 B
#6166	`StartFinishScope`	net6.0	564ns	0.746ns	2.89ns	0.00972	696 B
#6166	`StartFinishScope`	netcoreapp3.1	734ns	0.615ns	2.38ns	0.00931	696 B
#6166	`StartFinishScope`	net472	819ns	1.41ns	5.45ns	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	612ns	0.778ns	3.01ns	0.0098	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	941ns	1.79ns	6.94ns	0.00942	696 B
master	`RunOnMethodBegin`	net472	1.12μs	2.26ns	8.77ns	0.104	658 B
#6166	`RunOnMethodBegin`	net6.0	637ns	0.695ns	2.69ns	0.00987	696 B
#6166	`RunOnMethodBegin`	netcoreapp3.1	939ns	2.19ns	8.21ns	0.00938	696 B
#6166	`RunOnMethodBegin`	net472	1.07μs	0.772ns	2.68ns	0.105	658 B

@bouwkast

## Summary of changes added the code of Microsoft.OpenApi to the tracer, removed/changed the parts that require more recent versions of the language/fwk than we have, and updated generated files. ## Reason for change It's going to be needed for the protobuf instrumentation in #6166, and I'm doing it in a separate PR to limit the size of the diff in the actual change. ## Implementation details @bouwkast ran the vendoring tool to add the files, since it's not working on Mac apparently ## Test coverage ## Other details When looking at the size of artefacts published on the CI, it looks like this is increasing the size of the dll by ~180KiB  --------- Co-authored-by: Steven Bouwkamp <[email protected]>

…the items property

datadog-ddstaging · 2025-01-23T14:44:27Z

Datadog Report

Branch report: vandonr/proto
Commit report: 8ab2def
Test service: dd-trace-dotnet

✅ 0 Failed, 573075 Passed, 5449 Skipped, 46h 6m 5.07s Total Time

bouwkast

👍

Mostly just nitpicks and a couple test questions

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Protobuf/SchemaExtractor.cs

bouwkast · 2025-02-04T20:02:56Z

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Protobuf/SchemaExtractor.cs

+        ///  - the type ID as defined in https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/type.proto
+        ///  - the zero-based depth (allowing to distinguish a nested type)
+        /// If a field references a sub-message, that sub-message is expended first, before the current field is added to the hash.
+        /// Extraction stops after 10 nested messages, this affects the hash, so it must be kept consistent across tracers.


How do we plan to enforce this? Do we have system tests that cover it?
It doesn't appear to be configurable here so that seems good.
Would it be best to have a link to locations in the other tracers where this hashing is calculated?

yes I was thinking that it's pretty brittle as I was writing it. The only mechanism I know to ensure consistency between tracers are system tests, we should probably write one with a proto schema hitting as many branches as possible and use that to enforce this rule.

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Protobuf/SchemaExtractor.cs

tracer/src/Datadog.Trace/DataStreamsMonitoring/RateLimiter.cs

tracer/src/Datadog.Trace/Util/SmallCacheOrNoCache.cs

bouwkast · 2025-02-04T21:10:27Z

tracer/test/test-applications/integrations/Samples.GoogleProtobuf/Samples.GoogleProtobuf.csproj

@@ -0,0 +1,14 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <ApiVersion Condition="'$(ApiVersion)' == ''">3.28.2</ApiVersion>


Wondering if you tested with older versions as well?

Hmm, I don't think I did. Do you think I should just run it once locally, or add test cases ?

I was just thinking manually swapping it to an older version locally just to see that it works?

wow turns out testing on older versions is super annoying because you have to use the protobuf generator (protoc) that matches the version of protobuf you are using.

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

tracer/test/test-applications/integrations/Samples.GoogleProtobuf/sample.proto

+    WORK = 3;
+  }
+
+  message PhoneNumber {


How feasible would it be to test the path where we hit the limits for the number of properties and/or the extraction depth?

number of properties idk, I think it's something that we'd rarely encounter, and I'm not even sure my code is 100% aligned with what Java does.
The extraction depth is tested indirectly in the protobuf exploration tests, and I think we should test it in a system test, not sure it adds a lot of value to also test it here

Co-authored-by: Steven Bouwkamp <[email protected]>

This reverts commit 02d20a4.

## Summary of changes This reverts commit 02d20a4. ## Reason for change This has broken the master build, so reverting to unblock the pipeline ## Implementation details ## Test coverage ## Other details

This reverts commit 344c304.

@bouwkast

## Summary of changes same as #6166 except with: - fixes by @bouwkast from #6640 - gave up on generating the proto object file on the fly - fixed a couple inconsistency issues I discovered when system-testing vs the Java instrumentation (depth calculation was one-off, int value of types were not in sync, and we were not using the full name for references) --------- Co-authored-by: Steven Bouwkamp <[email protected]>

vandonr marked this pull request as ready for review December 18, 2024 16:29

vandonr requested review from a team as code owners December 18, 2024 16:29

vandonr force-pushed the vandonr/proto branch from 40cb1af to daf650f Compare December 19, 2024 09:48

vandonr changed the base branch from master to vandonr/vendored December 19, 2024 09:58

vandonr mentioned this pull request Dec 30, 2024

Vendor in Microsoft.OpenApi #6461

Merged

vandonr added type:bug area:tests unit tests, integration tests area:integrations and removed type:bug area:tests unit tests, integration tests labels Dec 30, 2024

vandonr force-pushed the vandonr/proto branch from 41f0d81 to 209f25c Compare January 10, 2025 12:54

Base automatically changed from vandonr/vendored to master January 14, 2025 10:41

vandonr force-pushed the vandonr/proto branch from 72fd7b2 to 164139c Compare January 14, 2025 11:06

vandonr added 5 commits January 14, 2025 12:08

base instrumentation for protobuf

6fe7a3e

sampling and caching

f3ad3b5

factorize the db connections cache into something reusable

e6b6fd9

add integration tests with sample

7900a34

bunch of fixes

d2f7ece

vandonr force-pushed the vandonr/proto branch from 164139c to d2f7ece Compare January 14, 2025 11:14

vandonr added 2 commits January 14, 2025 13:59

RegenerateSolutions

5bf817a

verbose comment explaining hash computation strategy

5527ec3

vandonr mentioned this pull request Jan 14, 2025

Change hash computation for protobuf to better represent impacting changes + save proto number in schema DataDog/dd-trace-java#8201

Merged

write protobuf number of arrays on the array directly rather than on …

4a8f252

…the items property

vandonr and others added 6 commits January 16, 2025 15:16

update doc on duck typing static types to account for #nullable checks

23a1648

exclude legacy tests on manually modified generated code

b05a8cf

Merge branch 'master' into vandonr/proto

6107e87

disable proto instrumentation in grpc tests

3236d38

use struct duck proxies when possible

664ee9e

Merge remote-tracking branch 'origin/master' into vandonr/proto

4aaaa0f

vandonr added 2 commits January 30, 2025 15:22

regen solution

734c00f

Merge remote-tracking branch 'origin/master' into vandonr/proto

02b54da

bouwkast reviewed Feb 4, 2025

View reviewed changes

bouwkast self-requested a review February 4, 2025 21:12

bouwkast approved these changes Feb 4, 2025

View reviewed changes

vandonr and others added 7 commits February 5, 2025 11:04

compare with OrdinalIgnoreCase

f93778e

Co-authored-by: Steven Bouwkamp <[email protected]>

update comment

da79442

Co-authored-by: Steven Bouwkamp <[email protected]>

address comments

7eb3ff9

flip checks on internal google files and tag

7f31b12

rename method to avoid confusion

4835fd5

trying something: generate proto code on the fly

5570e64

add comments

9da4b7f

vandonr force-pushed the vandonr/proto branch from ced8ee6 to 9da4b7f Compare February 6, 2025 10:31

reference SampleHelpers rather than Trace.Manual

8ab2def

vandonr merged commit 02d20a4 into master Feb 7, 2025
141 of 144 checks passed

vandonr deleted the vandonr/proto branch February 7, 2025 15:35

github-actions bot added this to the vNext-v3 milestone Feb 7, 2025

andrewlock added a commit that referenced this pull request Feb 10, 2025

Revert "Add Google Protobuf Instrumentation (#6166)"

fdab494

This reverts commit 02d20a4.

vandonr added a commit that referenced this pull request Feb 10, 2025

Reapply "Add Google Protobuf Instrumentation (#6166)"

92cbb4a

This reverts commit 344c304.

vandonr mentioned this pull request Feb 10, 2025

Reapply "Add Google Protobuf Instrumentation" #6647

Merged

andrewlock added the type:new-feature label Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Google Protobuf Instrumentation #6166

Add Google Protobuf Instrumentation #6166

vandonr commented Oct 18, 2024 •

edited

Loading

datadog-ddstaging bot commented Oct 18, 2024 •

edited

Loading

andrewlock commented Oct 18, 2024 •

edited

Loading

andrewlock commented Oct 18, 2024 •

edited

Loading

andrewlock commented Oct 18, 2024 •

edited

Loading

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #6166

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6166

Raw results

Raw results

datadog-ddstaging bot commented Jan 23, 2025 •

edited

Loading

bouwkast left a comment

bouwkast Feb 4, 2025

vandonr Feb 5, 2025

bouwkast Feb 4, 2025

vandonr Feb 5, 2025

bouwkast Feb 5, 2025

vandonr Feb 5, 2025

bouwkast Feb 4, 2025

vandonr Feb 5, 2025

Add Google Protobuf Instrumentation #6166

Add Google Protobuf Instrumentation #6166

Conversation

vandonr commented Oct 18, 2024 • edited Loading

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

datadog-ddstaging bot commented Oct 18, 2024 • edited Loading

Datadog Report

andrewlock commented Oct 18, 2024 • edited Loading

Execution-Time Benchmarks Report ⏱️

andrewlock commented Oct 18, 2024 • edited Loading

Throughput/Crank Report ⚡

andrewlock commented Oct 18, 2024 • edited Loading

Benchmarks Report for tracer 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #6166

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6166

Raw results

Raw results

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

Datadog Report

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

vandonr commented Oct 18, 2024 •

edited

Loading

datadog-ddstaging bot commented Oct 18, 2024 •

edited

Loading

andrewlock commented Oct 18, 2024 •

edited

Loading

andrewlock commented Oct 18, 2024 •

edited

Loading

andrewlock commented Oct 18, 2024 •

edited

Loading

datadog-ddstaging bot commented Jan 23, 2025 •

edited

Loading