[Testing] Improve span metadata rules by zacharycmontoya · Pull Request #3226 · DataDog/dd-trace-dotnet

zacharycmontoya · 2022-09-16T21:46:17Z

Summary of changes

A whole bunch of improvements to the span metadata rules infra in our integration test code.

Reason for change

Currently the span metadata rules are not required when adding a new integration and I believe they help make strong checks on the spans we produce.

Implementation details

Make span assertions more strict to raise an error when unexpected tags are present, and fix the rules that began failing with the stricter checks
Add a SpanMetadataRule entries for the new HotChocolate and Process integrations
Add ability to exclude tags from assertions (e.g. header tags added from tracer configuration)
Require each integration test to implement a SpanMetadataRule by making them derive from a new TracingIntegrationTest class
Add a GitHub Action to enforce that docs/span_metadata.md is up-to-date with tracer/test/Datadog.Trace.TestHelpers/SpanMetadataRules.cs

Test coverage

For integration tests, I ran as many tests locally as I could.

For the GitHub Action, you can see it fail before updating the markdown and then succeed after updating the markdown.

Other details

N/A

…t accounted for - Add rules for HotChocolate and Process integrations - Fix rules for other integrations. There are still some that fail for now...

…ssing

…ontract: ValidationIntegrationSpan(MockSpan) => Result - Apply the new abstract class to a subset of AspNetCore and Grpc test classes

…ionTest and implement the ValidateIntegrationSpan method

This reverts commit 206a2d2.

This reverts commit bba1802.

robertpi · 2022-09-19T09:15:44Z

+        {
+        }
+
+        public abstract Result ValidateIntegrationSpan(MockSpan span);


Anyway that this could be called automatically for each tests that inherits from this class? From a Dispose method maybe? I guess XUnit is not too helpful here ...

Yeah, nice idea, but not sure that's doable with unfortunately - IIRC, the Dispose method runs outside the context of a test, so failures there wouldn't be reported properly 🤔 Plus the mechanics of passing the spans + expectations etc are a bit problematic

robertpi

Couple of minor comments, but otherwise LGTM.

…nstructor to the correct order after removing extra compiler directives

…n method in all applicable tracing integration tests to maximize code reuse

…tually run CI on CosmosDb right now

pierotibou

Really nice.
One super small nit in the action

pierotibou · 2022-09-22T11:08:03Z

+          if ($LASTEXITCODE -eq 1) {
+            git diff -- .\docs\span_metadata.md
+            Write-Error "Found changes in docs/span_metadata.md file. Run build task GenerateSpanDocumentation to regenerate the file with the latest C# rules."
+            Exit 1


Do you plan to make this step mandatory in the repo?

Hmm I can update our Automatic Instrumentation docs to add this step for the integration development, as that's the only time you'll usually need to update this. Implemented in c59488f

I suspect Pierre meant marking the GitHub action as required for merges 🙂

I suspect Pierre meant marking the GitHub action as required for merges 🙂

Yep. Sorry if it wasn't clear

Co-authored-by: Pierre Bonet <[email protected]>

…taRules

andrewlock · 2022-09-22T16:38:15Z

Benchmarks Report 🐌

Benchmarks for #3226 compared to master:

All benchmarks have the same speed
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.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	717μs	452ns	1.75μs	0.355	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	458μs	231ns	866ns	0	2.59 KB
#3226	`WriteAndFlushEnrichedTraces`	net472	714μs	513ns	1.99μs	0.363	3.18 KB
#3226	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	453μs	115ns	416ns	0	2.59 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	199ns	0.232ns	0.898ns	0.0675	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	254ns	0.2ns	0.775ns	0.00583	0	424 B
master	`AllCycleMoreComplexBody`	net472	195ns	0.129ns	0.498ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	260ns	0.859ns	3.33ns	0.00543	0	400 B
master	`BodyExtractorSimpleBody`	net472	274ns	0.0902ns	0.338ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	235ns	0.312ns	1.21ns	0.00365	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	16.3μs	10.9ns	42.2ns	1.2	0.0163	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.6μs	5.09ns	18.3ns	0.0871	0	6.75 KB
#3226	`AllCycleSimpleBody`	net472	197ns	0.0439ns	0.158ns	0.0676	0	425 B
#3226	`AllCycleSimpleBody`	netcoreapp3.1	255ns	0.295ns	1.14ns	0.00574	0	424 B
#3226	`AllCycleMoreComplexBody`	net472	206ns	0.0679ns	0.263ns	0.0637	0	401 B
#3226	`AllCycleMoreComplexBody`	netcoreapp3.1	251ns	0.171ns	0.662ns	0.00538	0	400 B
#3226	`BodyExtractorSimpleBody`	net472	274ns	0.0987ns	0.369ns	0.0573	0	361 B
#3226	`BodyExtractorSimpleBody`	netcoreapp3.1	233ns	0.0751ns	0.291ns	0.00363	0	272 B
#3226	`BodyExtractorMoreComplexBody`	net472	16μs	13.3ns	51.6ns	1.2	0.0159	7.62 KB
#3226	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.4μs	6.67ns	25ns	0.0865	0	6.75 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	182μs	225ns	872ns	0.272	20.59 KB
#3226	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3226	`SendRequest`	netcoreapp3.1	180μs	295ns	1.14μs	0.18	20.58 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.92μs	2.05ns	7.92ns	0.15	0.00096	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.36μs	0.46ns	1.78ns	0.0129	0	936 B
#3226	`ExecuteNonQuery`	net472	1.89μs	1.19ns	4.59ns	0.15	0	947 B
#3226	`ExecuteNonQuery`	netcoreapp3.1	1.43μs	0.328ns	1.14ns	0.0122	0	936 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.66μs	1.57ns	6.08ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.56μs	0.635ns	2.29ns	0.0148	1.1 KB
master	`CallElasticsearchAsync`	net472	2.74μs	2.72ns	10.5ns	0.205	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.64μs	0.6ns	2.32ns	0.0164	1.22 KB
#3226	`CallElasticsearch`	net472	2.72μs	1.97ns	7.39ns	0.184	1.16 KB
#3226	`CallElasticsearch`	netcoreapp3.1	1.59μs	1.17ns	4.21ns	0.0149	1.1 KB
#3226	`CallElasticsearchAsync`	net472	2.77μs	1.58ns	6.13ns	0.205	1.29 KB
#3226	`CallElasticsearchAsync`	netcoreapp3.1	1.64μs	0.896ns	3.23ns	0.0164	1.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.86μs	1.63ns	6.33ns	0.225	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.8μs	0.6ns	2.25ns	0.018	1.34 KB
#3226	`ExecuteAsync`	net472	2.81μs	1.1ns	3.96ns	0.225	1.42 KB
#3226	`ExecuteAsync`	netcoreapp3.1	1.71μs	0.847ns	3.17ns	0.0179	1.34 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.84μs	3.65ns	13.7ns	0.439	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.74μs	1.66ns	6.23ns	0.0355	2.6 KB
#3226	`SendAsync`	net472	5.89μs	2.49ns	9.63ns	0.439	2.77 KB
#3226	`SendAsync`	netcoreapp3.1	3.77μs	3.55ns	12.3ns	0.0345	2.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.18μs	2.57ns	9.61ns	0.287	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.66μs	2.19ns	8.19ns	0.0255	1.85 KB
#3226	`EnrichedLog`	net472	3.36μs	6.11ns	23.7ns	0.287	1.81 KB
#3226	`EnrichedLog`	netcoreapp3.1	2.54μs	1.65ns	5.93ns	0.0256	1.85 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	157μs	98.5ns	381ns	0.696	0.232	4.66 KB
master	`EnrichedLog`	netcoreapp3.1	121μs	143ns	555ns	0.0603	0	4.5 KB
#3226	`EnrichedLog`	net472	154μs	250ns	970ns	0.694	0.231	4.65 KB
#3226	`EnrichedLog`	netcoreapp3.1	118μs	174ns	652ns	0.0586	0	4.49 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.9μs	2ns	7.74ns	0.57	0.00295	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.48μs	3.98ns	14.9ns	0.0533	0	3.91 KB
#3226	`EnrichedLog`	net472	5.98μs	3.73ns	14.4ns	0.57	0.00298	3.59 KB
#3226	`EnrichedLog`	netcoreapp3.1	4.56μs	1.62ns	5.85ns	0.0524	0	3.91 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.33μs	3.29ns	12.7ns	0.217	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.78μs	1.16ns	4.35ns	0.0177	1.32 KB
#3226	`SendReceive`	net472	2.41μs	6.13ns	22.9ns	0.217	1.37 KB
#3226	`SendReceive`	netcoreapp3.1	1.91μs	1.11ns	4.3ns	0.0181	1.32 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.04μs	3.43ns	13.3ns	0.352	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.38μs	2.04ns	7.9ns	0.0241	1.8 KB
#3226	`EnrichedLog`	net472	5.07μs	3.94ns	15.3ns	0.353	2.23 KB
#3226	`EnrichedLog`	netcoreapp3.1	4.28μs	1.85ns	6.93ns	0.0237	1.8 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`StartFinishSpan`	net472	1.23μs	0.57ns	2.13ns	0.128	0	810 B
master	`StartFinishSpan`	netcoreapp3.1	949ns	0.395ns	1.53ns	0.0105	0	760 B
master	`StartFinishScope`	net472	1.43μs	1.01ns	3.92ns	0.141	0	891 B
master	`StartFinishScope`	netcoreapp3.1	1.2μs	0.684ns	2.65ns	0.012	0	880 B
#3226	`StartFinishSpan`	net472	1.19μs	0.389ns	1.51ns	0.129	0.000587	810 B
#3226	`StartFinishSpan`	netcoreapp3.1	909ns	0.278ns	1.04ns	0.0105	0	760 B
#3226	`StartFinishScope`	net472	1.44μs	0.38ns	1.42ns	0.141	0	891 B
#3226	`StartFinishScope`	netcoreapp3.1	1.12μs	0.29ns	1.08ns	0.0117	0	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.49μs	1.05ns	4.08ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.13μs	0.704ns	2.54ns	0.0119	880 B
#3226	`RunOnMethodBegin`	net472	1.5μs	0.783ns	2.93ns	0.141	891 B
#3226	`RunOnMethodBegin`	netcoreapp3.1	1.2μs	1.48ns	5.74ns	0.0119	880 B

andrewlock · 2022-09-22T18:16:47Z

Code Coverage Report 📊

✔️ Merging #3226 into master will not change line coverage
✔️ Merging #3226 into master will not change branch coverage
✔️ Merging #3226 into master will not change complexity

	master	#3226	Change
Lines	`18009` / `24941`	`17975` / `24941`
Lines %	`72%`	`72%`	`0%` ✔️
Branches	`10488` / `15282`	`10481` / `15282`
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`16507`	`16507`	`0` ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3226	Change
Lines %	`72%`	`72%`	`0%` ✔️
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`16507`	`16507`	`0` ✔️

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️
Datadog.Trace.Activity.DiagnosticSourceEventListener	`-6%` ⛔	`0%` ✔️	`0` ✔️

View the full reports for further details:

zacharycmontoya added 8 commits September 16, 2022 09:05

- Strengthen span assertions to raise an error when extra tags are no…

f49f742

…t accounted for - Add rules for HotChocolate and Process integrations - Fix rules for other integrations. There are still some that fail for now...

Add ability to exclude a set of tags from the SpanMetadataRules proce…

81afb28

…ssing

Use exclude tags and rules updates so AspNetCore and Grpc test pass

18f1e0f

- Create new TracingIntegrationTest abstract class to enforce a new c…

fb19807

…ontract: ValidationIntegrationSpan(MockSpan) => Result - Apply the new abstract class to a subset of AspNetCore and Grpc test classes

Refactor majority of integration tests to derive from TracingIntegrat…

b309b41

…ionTest and implement the ValidateIntegrationSpan method

Update span metadata markdown

206a2d2

Revert "Update span metadata markdown"

bba1802

This reverts commit 206a2d2.

Add GitHub action to validate span_metadata.md is updated

844bbc1

zacharycmontoya added the area:tests unit tests, integration tests label Sep 16, 2022

zacharycmontoya self-assigned this Sep 16, 2022

zacharycmontoya requested review from a team as code owners September 16, 2022 21:46

zacharycmontoya added 2 commits September 16, 2022 14:47

Small update to GH Action

e4264cc

Revert "Revert "Update span metadata markdown""

166b4a0

This reverts commit bba1802.

This comment has been minimized.

Sign in to view

Fix WebRequest rule for .NET Core requests

1c216a9

This comment has been minimized.

Sign in to view

Update span metadata after test fixes. Thanks GitHub Actions!

2648ecb

This comment has been minimized.

Sign in to view

robertpi reviewed Sep 19, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.TestHelpers/SpanMetadataRulesHelpers.cs Outdated

robertpi reviewed Sep 19, 2022

View reviewed changes

robertpi approved these changes Sep 19, 2022

View reviewed changes

andrewlock approved these changes Sep 20, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/AdoNet/MicrosoftDataSqliteTests.cs Outdated

zacharycmontoya added 3 commits September 20, 2022 17:44

Move Datadog.Trace.TestHelpers.Result to its own file and move the co…

c0dec27

…nstructor to the correct order after removing extra compiler directives

Add TracingIntegrationTest.ValidateIntegrationSpans and use the commo…

748f445

…n method in all applicable tracing integration tests to maximize code reuse

Fix bad CosmosDb rule, which flew under the radar because we don't ac…

628c6a3

…tually run CI on CosmosDb right now

This comment has been minimized.

Sign in to view

Fix build issues

30162fb

This comment has been minimized.

Sign in to view

pierotibou approved these changes Sep 22, 2022

View reviewed changes

zacharycmontoya and others added 2 commits September 22, 2022 08:50

Update .github/workflows/verify_span_metadata_markdown_is_updated.yml

62d5ba5

Co-authored-by: Pierre Bonet <[email protected]>

Update automatic instrumentation steps to include usage of SpanMetada…

c59488f

…taRules

zacharycmontoya merged commit d6810f5 into master Sep 22, 2022

zacharycmontoya deleted the zach/innovation/enforce-span-metadata-rules branch September 22, 2022 19:04

github-actions Bot added this to the vNext milestone Sep 22, 2022

Uh oh!

Conversation

zacharycmontoya commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

robertpi Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertpi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pierotibou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pierotibou Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

pierotibou Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Sep 22, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

andrewlock commented Sep 22, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zacharycmontoya commented Sep 16, 2022 •

edited

Loading

andrewlock Sep 20, 2022 •

edited

Loading