Ensure we test a consistent commit for PRs by andrewlock · Pull Request #2649 · DataDog/dd-trace-dotnet

andrewlock · 2022-04-07T08:15:49Z

Summary of changes

For pull requests, don't use the built-in clone command in Azure Devops, as it uses the variable "merge" commit. Instead, clone the branch, and do the merge manually.

Reason for change

Merging PRs to master can currently cause unrelated PR builds to fail, because the commit being built changes for later stages.

Implementation details

By default, Azure Devops clones the special "merge" commit the GitHub creates for PRs. As we clone the repo multiple times (generally, once for each stage), that means later stages can end up cloning a different commit, if there have been commits to master since the first stages run. This often causes failures if those new commits introduce additional tests, or generally change anything.

To ensure we test all stages against the same commit, we make a note of the current commit ID of master before starting the pipeline. If we're running a branch directly, then we use the built-in clone command, so this change is a noop. If we're running a PR however, we clone the PR branch and then merge with the original commit ID manually. This ensures we are always testing the same effective merge commit.

The main downside is the additional complexity required in the pipeline, but what's a 100 extra lines in a 2000 line yaml file between friends 😬

Test coverage

Ran test branches to confirm it does the merge correctly + it doesn't merge when not doing a PR

Other details

Alternative approaches considered:

Only doing the checkout once, and using tar to pass around artifacts. A very good alternative. I thought this approach was going to be easier, but we run into drive space issues from "duplicating" everything on disk.
Moving all build stages to a single stage. Currently hard to do without a big impact on ci throughput, but would be ideal.

By default, Azure Devops clones the special "merge" commit the GitHub creates for PRs. As we clone the repo multiple times (generally, once for each stage), that means later stages can end up cloning a different commit, if there have been commits to master since the first stages run. This often causes failures if those new commits introduce additional tests, or generally change anything. To ensure we test all stages against the same commit, we make a note of the current commit ID of master before starting the pipeline. If we're running a branch directly, then we use the built-in clone command, so this change is a noop. If we're running a PR however, we clone the PR branch and then merge with the original commit ID _manually_. This ensures we are always testing the same effective merge commit. The main downside is the additional complexity required in the pipeline, but what's a 100 extra lines in a 2000 line yaml file between friends...

andrewlock · 2022-04-07T09:29:23Z

Code Coverage Report 📊

⚠️ Merging #2649 into master will will decrease line coverage by 1%
⚠️ Merging #2649 into master will will decrease branch coverage by 1%
✔️ Merging #2649 into master will not change complexity

	master	#2649	Change
Lines	`13480` / `18464`	`13371` / `18464`
Lines %	`73%`	`72%`	`-1%` ⚠️
Branches	`7798` / `11017`	`7732` / `11017`
Branches %	`71%`	`70%`	`-1%` ⚠️
Complexity	`12307`	`12307`	`0` ✔️

View the full report for further details:

Datadog.Trace Breakdown ⚠️

	master	#2649	Change
Lines %	`73%`	`72%`	`-1%` ⚠️
Branches %	`71%`	`70%`	`-1%` ⚠️
Complexity	`12307`	`12307`	`0` ✔️

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Telemetry.JsonWebRequestTelemetryTransport	`-27%` ⛔	`-57%` ⛔	`0` ✔️
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️
Datadog.Trace.Ci.CIVisibility	`7%` ✔️	`7%` ✔️	`0` ✔️

View the full reports for further details:

andrewlock · 2022-04-07T09:34:44Z

Benchmarks Report 🐌

Benchmarks for #2649 compared to master:

2 benchmarks are faster, with geometric mean 1.159
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	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	915μs	4.64μs	22.7μs	3.16 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	703μs	2.46μs	9.53μs	2.57 KB
#2649	`WriteAndFlushEnrichedTraces`	net472	926μs	3.93μs	14.7μs	3.16 KB
#2649	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	689μs	1.85μs	6.91μs	2.57 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	293ns	0.914ns	3.3ns	0.0519	0	329 B
master	`AllCycleSimpleBody`	netcoreapp3.1	364ns	1.18ns	4.56ns	0.00457	0	328 B
master	`AllCycleMoreComplexBody`	net472	287ns	1.01ns	3.63ns	0.0481	0	305 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	371ns	2.11ns	15.1ns	0.00423	0	304 B
master	`BodyExtractorSimpleBody`	net472	415ns	1.45ns	5.61ns	0.0569	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	422ns	1.97ns	7.88ns	0.00371	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	24μs	119ns	530ns	1.19	0.0118	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	21μs	98.6ns	406ns	0.0916	0	6.75 KB
#2649	`AllCycleSimpleBody`	net472	293ns	1.15ns	4.47ns	0.052	0	329 B
#2649	`AllCycleSimpleBody`	netcoreapp3.1	363ns	1.31ns	5.09ns	0.00458	0	328 B
#2649	`AllCycleMoreComplexBody`	net472	286ns	1.49ns	7.14ns	0.0482	0	305 B
#2649	`AllCycleMoreComplexBody`	netcoreapp3.1	370ns	1.43ns	5.35ns	0.00413	0	304 B
#2649	`BodyExtractorSimpleBody`	net472	422ns	2.08ns	12ns	0.057	0	361 B
#2649	`BodyExtractorSimpleBody`	netcoreapp3.1	438ns	1.69ns	6.98ns	0.00374	0	272 B
#2649	`BodyExtractorMoreComplexBody`	net472	23.7μs	112ns	449ns	1.18	0.0115	7.62 KB
#2649	`BodyExtractorMoreComplexBody`	netcoreapp3.1	21.7μs	83.5ns	344ns	0.095	0	6.75 KB

Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 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	295μs	758ns	2.73μs	0.146	19.74 KB
#2649	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#2649	`SendRequest`	netcoreapp3.1	301μs	1.54μs	7.05μs	0.28	19.74 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.73μs	4.2ns	16.3ns	0.0943	0	594 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.49μs	4.45ns	16.7ns	0.00891	0	632 B
#2649	`ExecuteNonQuery`	net472	1.71μs	7.66ns	29.7ns	0.0939	0.000831	594 B
#2649	`ExecuteNonQuery`	netcoreapp3.1	1.49μs	4.48ns	17.4ns	0.00892	0	632 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`CallElasticsearch`	net472	2.64μs	8.69ns	33.7ns	0.127	0	803 B
master	`CallElasticsearch`	netcoreapp3.1	1.74μs	8.58ns	34.3ns	0.011	0	792 B
master	`CallElasticsearchAsync`	net472	2.85μs	13.9ns	66.8ns	0.148	0.00138	939 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.78μs	6.58ns	24.6ns	0.0121	0	912 B
#2649	`CallElasticsearch`	net472	2.7μs	14.1ns	67.7ns	0.126	0	803 B
#2649	`CallElasticsearch`	netcoreapp3.1	1.65μs	5.92ns	22.1ns	0.0105	0	792 B
#2649	`CallElasticsearchAsync`	net472	2.78μs	10.3ns	38.4ns	0.149	0.00141	939 B
#2649	`CallElasticsearchAsync`	netcoreapp3.1	1.83μs	8.04ns	31.1ns	0.0127	0	912 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteAsync`	net472	3.24μs	17.4ns	96.7ns	0.165	0	1.06 KB
master	`ExecuteAsync`	netcoreapp3.1	2.14μs	12.3ns	87.5ns	0.0144	0	1.03 KB
#2649	`ExecuteAsync`	net472	3.12μs	10ns	37.5ns	0.168	0.00154	1.06 KB
#2649	`ExecuteAsync`	netcoreapp3.1	2.02μs	5.8ns	21.7ns	0.0141	0	1.03 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	6.77μs	36.1ns	191ns	0.362	2.28 KB
master	`SendAsync`	netcoreapp3.1	4.69μs	17ns	65.9ns	0.0306	2.21 KB
#2649	`SendAsync`	net472	6.81μs	35.2ns	169ns	0.361	2.28 KB
#2649	`SendAsync`	netcoreapp3.1	4.77μs	22.7ns	122ns	0.0304	2.21 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.45μs	8.57ns	29.7ns	0.227	1.45 KB
master	`EnrichedLog`	netcoreapp3.1	3.18μs	14.4ns	55.8ns	0.0215	1.53 KB
#2649	`EnrichedLog`	net472	3.49μs	20.2ns	180ns	0.227	1.45 KB
#2649	`EnrichedLog`	netcoreapp3.1	3.02μs	12ns	46.6ns	0.0206	1.53 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	292μs	821ns	3.18μs	0.437	0.146	4.33 KB
master	`EnrichedLog`	netcoreapp3.1	230μs	862ns	3.23μs	0	0	4.21 KB
#2649	`EnrichedLog`	net472	289μs	935ns	3.62μs	0.427	0.142	4.33 KB
#2649	`EnrichedLog`	netcoreapp3.1	232μs	744ns	2.78μs	0	0	4.21 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	7.65μs	32ns	128ns	0.505	3.23 KB
master	`EnrichedLog`	netcoreapp3.1	6.49μs	22.6ns	87.7ns	0.0508	3.6 KB
#2649	`EnrichedLog`	net472	7.82μs	32.9ns	123ns	0.507	3.23 KB
#2649	`EnrichedLog`	netcoreapp3.1	6.4μs	20.9ns	81ns	0.0499	3.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`SendReceive`	net472	2.42μs	9.69ns	39.9ns	0.16	0	1.01 KB
master	`SendReceive`	netcoreapp3.1	2.02μs	7.55ns	28.3ns	0.014	0	1.01 KB
#2649	`SendReceive`	net472	2.36μs	9.78ns	43.7ns	0.16	0.00115	1.01 KB
#2649	`SendReceive`	netcoreapp3.1	2μs	6.12ns	22.9ns	0.0138	0	1.01 KB

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

Faster 🎉 in #2649

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SerilogBenchmark.EnrichedLog‑netcoreapp3.1	1.154	5,830.03	5,053.13

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.93μs	26.5ns	103ns	0.29	1.87 KB
master	`EnrichedLog`	netcoreapp3.1	5.83μs	32.8ns	236ns	0.0196	1.49 KB
#2649	`EnrichedLog`	net472	6.01μs	21.4ns	80.2ns	0.29	1.87 KB
#2649	`EnrichedLog`	netcoreapp3.1	5.08μs	22.9ns	82.5ns	0.0202	1.49 KB

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

Faster 🎉 in #2649

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑netcoreapp3.1	1.165	1,199.03	1,029.20

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	926ns	4.94ns	27ns	0.0719	457 B
master	`StartFinishSpan`	netcoreapp3.1	919ns	4.14ns	15.5ns	0.00612	456 B
master	`StartFinishScope`	net472	1.1μs	5.58ns	26.2ns	0.0845	538 B
master	`StartFinishScope`	netcoreapp3.1	1.2μs	3.96ns	15.3ns	0.00777	576 B
#2649	`StartFinishSpan`	net472	899ns	3.69ns	13.8ns	0.0719	457 B
#2649	`StartFinishSpan`	netcoreapp3.1	892ns	4.93ns	29.2ns	0.00601	456 B
#2649	`StartFinishScope`	net472	1.09μs	3.47ns	13ns	0.0844	538 B
#2649	`StartFinishScope`	netcoreapp3.1	1.03μs	4.93ns	20.9ns	0.00805	576 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.27μs	3.37ns	12.6ns	0.0842	538 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.16μs	3.85ns	14.9ns	0.00807	576 B
#2649	`RunOnMethodBegin`	net472	1.28μs	6.13ns	25.3ns	0.0838	538 B
#2649	`RunOnMethodBegin`	netcoreapp3.1	1.16μs	4.51ns	17.4ns	0.0079	576 B

kevingosse

That looks... correct I guess?

kevingosse · 2022-04-08T11:43:12Z

+      git checkout --force $prBranch
+    displayName: checkout
+  - bash: |
+      git config --global user.email "[email protected]"


You could at least have made it @datadoghq.com 😛

I genuinely did originally, and decided not to do that, in case we add restrictions for datadoghq.com, and we accidentally end up pushing some code from here 😆

andrewlock added the area:builds project files, build scripts, pipelines, versioning, releases, packages label Apr 7, 2022

andrewlock requested a review from a team as a code owner April 7, 2022 08:15

kevingosse approved these changes Apr 8, 2022

View reviewed changes

pierotibou approved these changes Apr 11, 2022

View reviewed changes

andrewlock merged commit 6ea950e into master Apr 11, 2022

andrewlock deleted the andrew/ci/consistent-checkout branch April 11, 2022 08:35

github-actions Bot added this to the vNext milestone Apr 11, 2022

Uh oh!

Conversation

andrewlock commented Apr 7, 2022

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

andrewlock commented Apr 7, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ⚠️

Uh oh!

andrewlock commented Apr 7, 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

Faster 🎉 in #2649

Raw results

Faster 🎉 in #2649

Raw results

Raw results

Uh oh!

kevingosse left a comment

Choose a reason for hiding this comment

Uh oh!

kevingosse Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants