Improve logging output of automated package bumps #5909
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This project isn't a real project so the warning is not important.
From the docs it seems that reviewers is expected to be just GitHub user names.
Manually verified this locally. These tests aren't run on CI.
These are cases where there are typically hardcoded assembly versions in the instrumentations.
Note that tracing-dotnet was added previously due to CODEOWNERS.
bouwkast
commented
Aug 15, 2024
| @@ -436,7 +436,7 @@ | |||
| "SampleProjectName": "Samples.CosmosDb", | |||
| "NugetPackageSearchName": "Microsoft.Azure.Cosmos", | |||
| "MinVersion": "3.6.0", | |||
| "MaxVersionExclusive": "4.0.0", | |||
| "MaxVersionExclusive": "3.42.1", | |||
There was a problem hiding this comment.
I tested version 3.42.0 locally.
The snapshots aren't entirely out of date as some git tags have been scrubbed from all snapshots but besides that it looked good, will make another PR to update those snapshots though.
There was a problem hiding this comment.
🤔 Is this how we want it to work 🤔 I don't know if the updater would notify about new versions in that case? I'll take your word for it that this is how you want it to work though 🙂
Another possibility - we add a flag to the pipeline to make it test cosmos on these PRs (version bump) only. We only don't test it in CI because the infra is really flaky IIRC, but that doesn't matter so much if it only affects these PRs 🤷♂️
There was a problem hiding this comment.
So it is how the Azure.Messaging.ServiceBus is currently setup as well, my thoughts is that hopefully when Dependabot starts back up it should be able to keep on top of things as that should update it as well.
I do like the idea of adding that flag though, will look into that thanks!
bouwkast
commented
Aug 15, 2024
|
|
||
| <!-- Integration: Azure.Messaging.ServiceBus --> | ||
| <!-- Assembly: Azure.Messaging.ServiceBus --> | ||
| <!-- Latest package https://www.nuget.org/packages/Azure.Messaging.ServiceBus/7.18.1 --> | ||
| <!-- ⚠POTENTIALLY UNSUPPORTED⚠ Latest package https://www.nuget.org/packages/Azure.Messaging.ServiceBus/7.18.1 --> |
There was a problem hiding this comment.
I'll make a task of testing this out locally for v7.18.1 (done it before and I think Zach made steps on how to do it), but it isn't a priority ATM.
There was a problem hiding this comment.
Another possibility here (probably harder work, just food for thought)
- Set up an app that uses Service Bus that deploys to Azure
- Instrument with the latest dev version of the AAS extension
- Set up dependabot for servicebus on the new app so we can auto merge it
- Set up slack alerts in DD to warn if anything's changed on that test app
bouwkast
commented
Aug 15, 2024
| @@ -41,7 +41,7 @@ jobs: | |||
| base: master | |||
| title: "[Test Package Versions Bump] Updating package versions " | |||
| milestone: "${{steps.rename.outputs.milestone}}" | |||
| reviewers: "DataDog/apm-dotnet" | |||
| team-reviewers: "DataDog/apm-dotnet,DataDog/apm-idm-dotnet" | |||
There was a problem hiding this comment.
Documentation for this is here: https://github.com/peter-evans/create-pull-request?tab=readme-ov-file#action-inputs
This isn't super important as CODEOWNERS kicks in, but it'd be nice to get IDM on there.
bouwkast
commented
Aug 15, 2024
bouwkast
commented
Aug 15, 2024
| @@ -187,8 +187,8 @@ private async Task PopulatePackages(List<PackageVersionGenerator.TestedPackage> | |||
|
|
|||
| if (latestSupportedPackage is null) | |||
| { | |||
| Logger.Warning($"No version of {packageName} below maximum package version {MaximumSupportedAssemblyVersion}." + | |||
| $"Using latest instead"); | |||
| Logger.Warning($"No version of {packageName} below maximum package version {MaximumSupportedAssemblyVersion}. " + | |||
There was a problem hiding this comment.
I don't really understand exactly what these mean
For example the OpenTelemetry instrumentations hit this:
There was a problem hiding this comment.
Theoretically, this means that we only support (for example) 1.x.x-2.x.x, but the only package it can find is 3.x.x...
Generally it means the mapping between assemblies and nugets is a mess and we can't always infer stuff as we'd like
Datadog ReportBranch report: ✅ 0 Failed, 300946 Passed, 1597 Skipped, 12h 16m 18.37s Total Time |
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:
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). |
andrewlock
approved these changes
Aug 16, 2024
| @@ -436,7 +436,7 @@ | |||
| "SampleProjectName": "Samples.CosmosDb", | |||
| "NugetPackageSearchName": "Microsoft.Azure.Cosmos", | |||
| "MinVersion": "3.6.0", | |||
| "MaxVersionExclusive": "4.0.0", | |||
| "MaxVersionExclusive": "3.42.1", | |||
There was a problem hiding this comment.
🤔 Is this how we want it to work 🤔 I don't know if the updater would notify about new versions in that case? I'll take your word for it that this is how you want it to work though 🙂
Another possibility - we add a flag to the pipeline to make it test cosmos on these PRs (version bump) only. We only don't test it in CI because the infra is really flaky IIRC, but that doesn't matter so much if it only affects these PRs 🤷♂️
|
|
||
| <!-- Integration: Azure.Messaging.ServiceBus --> | ||
| <!-- Assembly: Azure.Messaging.ServiceBus --> | ||
| <!-- Latest package https://www.nuget.org/packages/Azure.Messaging.ServiceBus/7.18.1 --> | ||
| <!-- ⚠POTENTIALLY UNSUPPORTED⚠ Latest package https://www.nuget.org/packages/Azure.Messaging.ServiceBus/7.18.1 --> |
There was a problem hiding this comment.
Another possibility here (probably harder work, just food for thought)
- Set up an app that uses Service Bus that deploys to Azure
- Instrument with the latest dev version of the AAS extension
- Set up dependabot for servicebus on the new app so we can auto merge it
- Set up slack alerts in DD to warn if anything's changed on that test app
quinna-h
approved these changes
Aug 19, 2024
Co-authored-by: Lucas Pimentel <lucas.pimentel@datadoghq.com>
Benchmarks Report for tracer 🐌Benchmarks for #5909 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.ILoggerBenchmark.EnrichedLog‑netcoreapp3.1 | 1.207 | 2,012.73 | 2,428.65 |
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.ILoggerBenchmark.EnrichedLog‑net6.0 | 1.123 | 1,637.97 | 1,458.33 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 1.64μs | 0.963ns | 3.6ns | 0.023 | 0 | 0 | 1.64 KB |
| master | EnrichedLog |
netcoreapp3.1 | 2.01μs | 0.896ns | 3.35ns | 0.0222 | 0 | 0 | 1.64 KB |
| master | EnrichedLog |
net472 | 2.72μs | 2.69ns | 10.4ns | 0.249 | 0 | 0 | 1.57 KB |
| #5909 | EnrichedLog |
net6.0 | 1.46μs | 0.823ns | 3.19ns | 0.0225 | 0 | 0 | 1.64 KB |
| #5909 | EnrichedLog |
netcoreapp3.1 | 2.43μs | 1.32ns | 5.11ns | 0.0218 | 0 | 0 | 1.64 KB |
| #5909 | EnrichedLog |
net472 | 2.68μs | 1.48ns | 5.35ns | 0.249 | 0 | 0 | 1.57 KB |
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 115μs | 285ns | 1.11μs | 0.0567 | 0 | 0 | 4.28 KB |
| master | EnrichedLog |
netcoreapp3.1 | 119μs | 223ns | 833ns | 0.0597 | 0 | 0 | 4.28 KB |
| master | EnrichedLog |
net472 | 149μs | 155ns | 601ns | 0.667 | 0.222 | 0 | 4.46 KB |
| #5909 | EnrichedLog |
net6.0 | 117μs | 259ns | 1μs | 0 | 0 | 0 | 4.28 KB |
| #5909 | EnrichedLog |
netcoreapp3.1 | 121μs | 188ns | 677ns | 0.0606 | 0 | 0 | 4.28 KB |
| #5909 | EnrichedLog |
net472 | 150μs | 181ns | 677ns | 0.671 | 0.224 | 0 | 4.46 KB |
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 3.08μs | 0.724ns | 2.71ns | 0.0309 | 0 | 0 | 2.2 KB |
| master | EnrichedLog |
netcoreapp3.1 | 4.25μs | 1.81ns | 7.03ns | 0.0297 | 0 | 0 | 2.2 KB |
| master | EnrichedLog |
net472 | 4.83μs | 0.936ns | 3.38ns | 0.319 | 0 | 0 | 2.02 KB |
| #5909 | EnrichedLog |
net6.0 | 3.08μs | 0.636ns | 2.46ns | 0.031 | 0 | 0 | 2.2 KB |
| #5909 | EnrichedLog |
netcoreapp3.1 | 4.28μs | 1.87ns | 7.24ns | 0.03 | 0 | 0 | 2.2 KB |
| #5909 | EnrichedLog |
net472 | 4.76μs | 0.671ns | 2.51ns | 0.32 | 0 | 0 | 2.02 KB |
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendReceive |
net6.0 | 1.42μs | 0.767ns | 2.97ns | 0.0162 | 0 | 0 | 1.14 KB |
| master | SendReceive |
netcoreapp3.1 | 1.77μs | 1.32ns | 4.95ns | 0.015 | 0 | 0 | 1.14 KB |
| master | SendReceive |
net472 | 2.2μs | 1.69ns | 6.53ns | 0.184 | 0 | 0 | 1.16 KB |
| #5909 | SendReceive |
net6.0 | 1.3μs | 0.696ns | 2.51ns | 0.0158 | 0 | 0 | 1.14 KB |
| #5909 | SendReceive |
netcoreapp3.1 | 1.78μs | 0.668ns | 2.59ns | 0.0151 | 0 | 0 | 1.14 KB |
| #5909 | SendReceive |
net472 | 2.15μs | 1.54ns | 5.95ns | 0.183 | 0.00107 | 0 | 1.16 KB |
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 2.76μs | 1.38ns | 4.78ns | 0.0221 | 0 | 0 | 1.6 KB |
| master | EnrichedLog |
netcoreapp3.1 | 3.95μs | 1.17ns | 4.53ns | 0.0219 | 0 | 0 | 1.65 KB |
| master | EnrichedLog |
net472 | 4.6μs | 1.63ns | 6.33ns | 0.322 | 0 | 0 | 2.04 KB |
| #5909 | EnrichedLog |
net6.0 | 2.96μs | 1.15ns | 4.3ns | 0.0222 | 0 | 0 | 1.6 KB |
| #5909 | EnrichedLog |
netcoreapp3.1 | 3.99μs | 1.67ns | 6.46ns | 0.0221 | 0 | 0 | 1.65 KB |
| #5909 | EnrichedLog |
net472 | 4.4μs | 2.53ns | 9.79ns | 0.322 | 0 | 0 | 2.04 KB |
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #5909
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472
1.140
660.13
578.83
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net472
1.120
859.16
766.77
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 | 1.140 | 660.13 | 578.83 | |
| Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net472 | 1.120 | 859.16 | 766.77 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 400ns | 0.675ns | 2.61ns | 0.00802 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 547ns | 1.85ns | 6.69ns | 0.00772 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 660ns | 0.227ns | 0.878ns | 0.0916 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 513ns | 0.596ns | 2.31ns | 0.00981 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 741ns | 0.291ns | 1.13ns | 0.00933 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 859ns | 0.632ns | 2.36ns | 0.104 | 0 | 0 | 658 B |
| #5909 | StartFinishSpan |
net6.0 | 419ns | 0.808ns | 3.13ns | 0.00814 | 0 | 0 | 576 B |
| #5909 | StartFinishSpan |
netcoreapp3.1 | 560ns | 0.308ns | 1.19ns | 0.00789 | 0 | 0 | 576 B |
| #5909 | StartFinishSpan |
net472 | 579ns | 0.308ns | 1.19ns | 0.0916 | 0 | 0 | 578 B |
| #5909 | StartFinishScope |
net6.0 | 491ns | 0.287ns | 1.11ns | 0.00985 | 0 | 0 | 696 B |
| #5909 | StartFinishScope |
netcoreapp3.1 | 681ns | 0.657ns | 2.55ns | 0.00978 | 0 | 0 | 696 B |
| #5909 | StartFinishScope |
net472 | 767ns | 0.458ns | 1.59ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #5909
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0
1.122
673.62
600.17
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 | 1.122 | 673.62 | 600.17 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 673ns | 0.659ns | 2.38ns | 0.00958 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 942ns | 0.427ns | 1.65ns | 0.00901 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.12μs | 1.54ns | 5.55ns | 0.104 | 0 | 0 | 658 B |
| #5909 | RunOnMethodBegin |
net6.0 | 601ns | 0.306ns | 1.15ns | 0.00971 | 0 | 0 | 696 B |
| #5909 | RunOnMethodBegin |
netcoreapp3.1 | 861ns | 0.864ns | 3.35ns | 0.0095 | 0 | 0 | 696 B |
| #5909 | RunOnMethodBegin |
net472 | 1.07μs | 0.188ns | 0.728ns | 0.104 | 0 | 0 | 658 B |
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 (5909) (11.604M) : 0, 11604433
master (11.606M) : 0, 11606357
benchmarks/2.9.0 (11.883M) : 0, 11882814
section Automatic
This PR (5909) (7.784M) : 0, 7783649
master (7.756M) : 0, 7755721
benchmarks/2.9.0 (8.446M) : 0, 8445613
section Trace stats
master (8.110M) : 0, 8109885
section Manual
master (11.466M) : 0, 11465663
section Manual + Automatic
This PR (5909) (7.235M) : 0, 7234692
master (7.192M) : 0, 7192253
section DD_TRACE_ENABLED=0
master (10.665M) : 0, 10665360
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5909) (9.660M) : 0, 9660320
benchmarks/2.9.0 (9.760M) : 0, 9759697
section Automatic
This PR (5909) (6.592M) : 0, 6591602
section Manual + Automatic
This PR (5909) (6.264M) : 0, 6264143
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5909) (10.005M) : 0, 10005268
master (10.088M) : 0, 10088205
section Automatic
This PR (5909) (6.693M) : 0, 6692891
master (6.620M) : 0, 6619922
section Trace stats
master (7.305M) : 0, 7304619
section Manual
master (9.981M) : 0, 9980905
section Manual + Automatic
This PR (5909) (6.276M) : 0, 6276161
master (6.066M) : 0, 6066177
section DD_TRACE_ENABLED=0
master (9.345M) : 0, 9344849
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
This is a conglomeration of changes/improvements surrounding auto-package bumps with the main focus of just improving the logging output.
Reason for change
Trying to solve instrumentations potentially out of date and want to make sure that the visibility into potential issues is clear.
Coupled with #5908 to fix Dependabot (runs daily) we should be able to keep on top of updates fairly well barring the instrumentations that aren't tested in CI at the moment and require running locally.
Implementation details
<NoWarn>NU1701</NoWarn>to Honeypot - it isn't a real project so hopefully should be fineerror NU1202: Package Aerospike.Client 7.2.0 is not compatible with netcoreapp3.1that were encountered in Fix dependabot and improveGeneratePackageVersions#5579apm-idm-dotnetas reviewer to the Dependabot Honeypot PRs and to the Automated Test Project PR that happens every Sunday.PackageVersionsGeneratorDefinitions.jsonso that a warning will be added as its tests aren't run in CI. Note: this doesn't change the upper limit of the supported instrumentation.Test coverage
master🫤Other details
See #5908 for what should fix Dependabot.
Will make tasks to look into CosmosDb and
Azure.Messaging.ServiceBusto get clear instructions for IDM on how to test them locally and look into what it'll take to get them into CI.