Skip to content

Add NullDiscoveryService for CI visibility and testing#3253

Merged
andrewlock merged 2 commits into
masterfrom
andrew/disable-discovery-service-in-ciapp-agentless
Sep 23, 2022
Merged

Add NullDiscoveryService for CI visibility and testing#3253
andrewlock merged 2 commits into
masterfrom
andrew/disable-discovery-service-in-ciapp-agentless

Conversation

@andrewlock

@andrewlock andrewlock commented Sep 23, 2022

Copy link
Copy Markdown
Member

Summary of changes

  • Adds a noop NullDiscoveryService
  • Use NullDiscoveryService for CI visibility when in agentless mode
  • Use NullDiscoveryService by default with our internal new Tracer() constructor (used in tests)

Reason for change

  • The agent isn't available when running in ci-app in agentless mode, so no sense starting the discovery service, as it will never succeed
  • In tests that were creating a tracer, they were starting a discoveryservice instance. For unit-tests (where they run without an agent), this would inevitably fail, writing a log message. As we run hundreds of tests, this generates hundreds of logs. On top of that, as the discoveryservice wasn't shut down, it was retrying, ultimately generating thousands of logs across all the different tests.

Implementation details

Create a NullDiscoveryService and use it by default for the internal Tracer constructor. This is the same approach we use for Telemetry, and is a pretty safe change to make, as the "normal" API does not use this approach. Need to update any tests which expect the real DiscoveryService to be created (e.g. StatsTests)

Added GetDiscoveryService() overridable method to TracerManagerFactory so that ci-visibility can customise as required.

Test coverage

No explicit tests for NullDiscoveryService, just need to make sure all existing tests pass.

Other details

Hopefully we don't see any more log explosions in CI 🤞

@andrewlock andrewlock added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) area:tests unit tests, integration tests area:ci-visibility labels Sep 23, 2022
@andrewlock
andrewlock requested review from a team as code owners September 23, 2022 10:39
@andrewlock andrewlock changed the title Add NullDiscoveryService for ciapp and testing Add NullDiscoveryService for CI visibility and testing Sep 23, 2022
tonyredondo
tonyredondo approved these changes Sep 23, 2022
View reviewed changes

@tonyredondo tonyredondo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this!

@andrewlock

andrewlock commented Sep 23, 2022

Copy link
Copy Markdown
Member Author

Benchmarks Report 🐌

Benchmarks for #3253 compared to master:

  • 1 benchmarks are slower, with geometric mean 1.126
  • 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 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 720μs 926ns 3.34μs 0.355 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 453μs 365ns 1.41μs 0 0 0 2.58 KB
#3253 WriteAndFlushEnrichedTraces net472 716μs 708ns 2.65μs 0.355 0 0 3.18 KB
#3253 WriteAndFlushEnrichedTraces netcoreapp3.1 478μs 250ns 937ns 0 0 0 2.58 KB
Benchmarks.Trace.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #3253

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.AppSecBodyBenchmark.BodyExtractorSimpleBody‑netcoreapp3.1 1.126 232.49 261.75

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 197ns 0.0703ns 0.263ns 0.0676 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 255ns 0.269ns 1.04ns 0.00577 0 0 424 B
master AllCycleMoreComplexBody net472 196ns 0.063ns 0.218ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 253ns 0.153ns 0.573ns 0.00542 0 0 400 B
master BodyExtractorSimpleBody net472 280ns 0.138ns 0.517ns 0.0574 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 232ns 0.166ns 0.621ns 0.00372 0 0 272 B
master BodyExtractorMoreComplexBody net472 15.8μs 10.2ns 38.1ns 1.21 0.0158 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.3μs 5.06ns 19.6ns 0.0919 0 0 6.75 KB
#3253 AllCycleSimpleBody net472 199ns 0.0396ns 0.143ns 0.0675 0 0 425 B
#3253 AllCycleSimpleBody netcoreapp3.1 250ns 0.47ns 1.82ns 0.0059 0 0 424 B
#3253 AllCycleMoreComplexBody net472 203ns 0.0695ns 0.251ns 0.0637 0 0 401 B
#3253 AllCycleMoreComplexBody netcoreapp3.1 248ns 0.103ns 0.371ns 0.0055 0 0 400 B
#3253 BodyExtractorSimpleBody net472 278ns 0.123ns 0.458ns 0.0573 0 0 361 B
#3253 BodyExtractorSimpleBody netcoreapp3.1 262ns 0.15ns 0.541ns 0.00371 0 0 272 B
#3253 BodyExtractorMoreComplexBody net472 15.6μs 4.79ns 17.9ns 1.21 0.0157 0 7.62 KB
#3253 BodyExtractorMoreComplexBody netcoreapp3.1 12.7μs 4.58ns 17.1ns 0.089 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
master SendRequest netcoreapp3.1 181μs 245ns 918ns 0.271 0 0 20.59 KB
#3253 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3253 SendRequest netcoreapp3.1 180μs 188ns 727ns 0.268 0 0 20.58 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net472 1.86μs 1.22ns 4.39ns 0.15 0.000934 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.46μs 0.653ns 2.45ns 0.0124 0 0 936 B
#3253 ExecuteNonQuery net472 1.86μs 1ns 3.87ns 0.15 0.000927 0 947 B
#3253 ExecuteNonQuery netcoreapp3.1 1.35μs 0.553ns 2ns 0.0128 0 0 936 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net472 2.61μs 1.84ns 6.9ns 0.182 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.51μs 0.888ns 3.32ns 0.0144 0 0 1.1 KB
master CallElasticsearchAsync net472 2.72μs 1.65ns 6.4ns 0.205 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.59μs 0.64ns 2.48ns 0.0166 0 0 1.22 KB
#3253 CallElasticsearch net472 2.55μs 1.06ns 3.95ns 0.183 0 0 1.16 KB
#3253 CallElasticsearch netcoreapp3.1 1.59μs 0.461ns 1.72ns 0.0152 0 0 1.1 KB
#3253 CallElasticsearchAsync net472 2.59μs 1.23ns 4.76ns 0.204 0 0 1.29 KB
#3253 CallElasticsearchAsync netcoreapp3.1 1.58μs 0.464ns 1.73ns 0.0162 0 0 1.22 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net472 2.9μs 2.49ns 9.63ns 0.225 0 0 1.42 KB
master ExecuteAsync netcoreapp3.1 1.8μs 0.916ns 3.43ns 0.0189 0 0 1.34 KB
#3253 ExecuteAsync net472 2.76μs 0.864ns 3.35ns 0.224 0 0 1.42 KB
#3253 ExecuteAsync netcoreapp3.1 1.74μs 0.836ns 3.24ns 0.0183 0 0 1.34 KB
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net472 5.88μs 5.5ns 21.3ns 0.44 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.78μs 1.53ns 5.91ns 0.0341 0 0 2.6 KB
#3253 SendAsync net472 5.84μs 1.08ns 4.19ns 0.438 0 0 2.77 KB
#3253 SendAsync netcoreapp3.1 3.66μs 1.39ns 5.38ns 0.0349 0 0 2.6 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 3.09μs 2.07ns 7.75ns 0.288 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.53μs 1.94ns 7.01ns 0.0254 0 0 1.85 KB
#3253 EnrichedLog net472 3.22μs 1.43ns 5.52ns 0.287 0 0 1.81 KB
#3253 EnrichedLog netcoreapp3.1 2.54μs 1ns 3.61ns 0.0252 0 0 1.85 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 net472 152μs 115ns 444ns 0.682 0.227 0 4.65 KB
master EnrichedLog netcoreapp3.1 115μs 189ns 733ns 0.0581 0 0 4.49 KB
#3253 EnrichedLog net472 151μs 213ns 825ns 0.682 0.227 0 4.65 KB
#3253 EnrichedLog netcoreapp3.1 115μs 101ns 391ns 0.0576 0 0 4.49 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 net472 5.97μs 5.03ns 18.1ns 0.568 0.00299 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.51μs 1.18ns 4.1ns 0.0518 0 0 3.91 KB
#3253 EnrichedLog net472 5.86μs 2.78ns 10.4ns 0.568 0.00294 0 3.59 KB
#3253 EnrichedLog netcoreapp3.1 4.44μs 2.28ns 8.54ns 0.0532 0 0 3.91 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 net472 2.3μs 3.86ns 15ns 0.217 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.79μs 1.29ns 4.84ns 0.018 0 0 1.32 KB
#3253 SendReceive net472 2.24μs 0.742ns 2.87ns 0.217 0 0 1.37 KB
#3253 SendReceive netcoreapp3.1 1.8μs 0.715ns 2.77ns 0.0172 0 0 1.32 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 net472 4.95μs 1.82ns 7.05ns 0.353 0.00247 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.31μs 2.17ns 7.83ns 0.0235 0 0 1.8 KB
#3253 EnrichedLog net472 5.03μs 1.69ns 6.56ns 0.354 0 0 2.23 KB
#3253 EnrichedLog netcoreapp3.1 4.22μs 1.5ns 5.62ns 0.0231 0 0 1.8 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net472 1.16μs 0.618ns 2.39ns 0.129 0 0 810 B
master StartFinishSpan netcoreapp3.1 943ns 0.322ns 1.2ns 0.0104 0 0 760 B
master StartFinishScope net472 1.49μs 2.73ns 10.6ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.04μs 0.404ns 1.57ns 0.012 0 0 880 B
#3253 StartFinishSpan net472 1.15μs 0.17ns 0.635ns 0.129 0 0 810 B
#3253 StartFinishSpan netcoreapp3.1 914ns 0.376ns 1.36ns 0.01 0 0 760 B
#3253 StartFinishScope net472 1.38μs 0.881ns 3.3ns 0.141 0 0 891 B
#3253 StartFinishScope netcoreapp3.1 1.05μs 0.349ns 1.31ns 0.0122 0 0 880 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net472 1.49μs 0.87ns 3.26ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.18μs 0.432ns 1.56ns 0.0118 0 0 880 B
#3253 RunOnMethodBegin net472 1.49μs 0.528ns 2.05ns 0.141 0 0 891 B
#3253 RunOnMethodBegin netcoreapp3.1 1.21μs 0.608ns 2.28ns 0.0121 0 0 880 B

@andrewlock

andrewlock commented Sep 23, 2022

Copy link
Copy Markdown
Member Author

Code Coverage Report 📊

✔️ Merging #3253 into master will not change line coverage
✔️ Merging #3253 into master will not change branch coverage
⛔ Merging #3253 into master will will increase complexity by 9

master #3253 Change
Lines 18039 / 24950 18058 / 24956
Lines % 72% 72% 0% ✔️
Branches 10523 / 15302 10534 / 15306
Branches % 69% 69% 0% ✔️
Complexity 16527 16536 9 ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3253 Change
Lines % 72% 72% 0% ✔️
Branches % 69% 69% 0% ✔️
Complexity 16527 16536 9 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Ci.CITracerManagerFactory 3% ✔️ 8% ✔️ 2 ⛔

The following classes were added in #3253:

File Line coverage Branch coverage Complexity
Datadog.Trace.Agent.DiscoveryService.NullDiscoveryService 25% 100% 4

View the full reports for further details:

@andrewlock
andrewlock merged commit c8431d8 into master Sep 23, 2022
@andrewlock
andrewlock deleted the andrew/disable-discovery-service-in-ciapp-agentless branch September 23, 2022 11:40
@github-actions github-actions Bot added this to the vNext milestone Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonyredondo tonyredondo tonyredondo approved these changes
@pierotibou pierotibou pierotibou approved these changes

Assignees

No one assigned

Labels

area:ci-visibility area:tests unit tests, integration tests area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations)

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

3 participants

@andrewlock @tonyredondo @pierotibou