Skip to content

[Tracer] Add DiscoveryService to Tracer initialization and update StatsAggregator to use Agent feature client_drop_p0s#3152

Merged
zacharycmontoya merged 8 commits into
masterfrom
zach/stats/discovery-update
Sep 8, 2022
Merged

[Tracer] Add DiscoveryService to Tracer initialization and update StatsAggregator to use Agent feature client_drop_p0s#3152
zacharycmontoya merged 8 commits into
masterfrom
zach/stats/discovery-update

Conversation

@zacharycmontoya

@zacharycmontoya zacharycmontoya commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

Summary of changes

Use the existing DiscoveryService to ping the /info endpoint of the agent and extract the property client_drop_p0s=<bool>. If the value is false, disable the stats computation feature, even if it was previously enabled.

Reason for change

If the agent does not support tracer's dropping P0 traces, then we should disable stats computation in the tracer because we will need to send all spans anyways so it doesn't make sense to compute stats points.

Implementation details

API changes

  • Add an IDiscoveryService parameter to the various StatsAggregator and AgentWriter methods needed to produce a StatsAggregator instance
  • Update IDiscoveryService with two new properties that are populated from the discovery request:
    • string StatsEndpoint
    • bool? ClientDropP0s

Startup logic

  1. When the Tracer.Instance is created (by user during custom instrumentation or Instrumentation.cs during automatic instrumentation), DiscoveryService.Create is invoked when initializing the TracerManager.
  2. When stats computation is enabled by the configuration, the DiscoveryService object is passed down to the
  3. The StatsAggregator constructor begins a Task that invokes discoveryService.DiscoverAsync(), and StatsAggregator.CanComputeStats will be asynchronously updated from null to true/false
  4. When automatic instrumentation is enabled, DiscoveryService.Create will be invoked and will return the cached DiscoveryService.Instance value, which will be used to initialize the Remote Configuration Management and Live Debugger products
  5. discoveryService.DiscoverAsync() will be invoked (as authored before) to finish the initialization of the two products

Note: Yes, with this logic, discoveryService.DiscoverAsync() will be invoked twice during the startup path of automatic instrumentation, but Andrew has ideas for improving this in a follow-up PR.

Test coverage

This PR adds two new test cases in StatsTests to ensure the following properties:

  1. The feature is disabled when the agent doesn't have the stats endpoint.
  2. The feature is disabled when the agent reports client_drop_p0s=false

All of the other tests run a MockTracerAgent that reports a stats endpoint and reports the feature client_drop_p0s=true, without any other mocking. Since these tests avoid mocking static dependencies, this required new test classes DiscoveryServiceInstanceTestCollection and DiscoveryServiceRestorerAttribute to manage the static DiscoveryService.Instance for these tests

Other details

Replaces #3049

@zacharycmontoya zacharycmontoya added type:enhancement Improvement to an existing feature area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) area:tests unit tests, integration tests labels Sep 1, 2022
@zacharycmontoya zacharycmontoya self-assigned this Sep 1, 2022
@zacharycmontoya
zacharycmontoya requested review from a team as code owners September 1, 2022 22:37
@zacharycmontoya zacharycmontoya mentioned this pull request Sep 1, 2022
Closed
@github-actions

This comment was marked as resolved.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@zacharycmontoya

zacharycmontoya commented Sep 2, 2022

Copy link
Copy Markdown
Contributor Author

The snapshot diffs might be a result of the WCF changes and the CI Code Coverage changes being merged recently. Importantly, the integration tests seem to pass so I guess it's okay?

@andrewlock andrewlock mentioned this pull request Sep 2, 2022
Merged
andrewlock
andrewlock approved these changes Sep 2, 2022
View reviewed changes

@andrewlock andrewlock 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.

LGTM, I'll base my discovery service changes on top of these

Comment thread tracer/src/Datadog.Trace/Ci/Agent/CIAgentWriter.cs Outdated
@andrewlock andrewlock mentioned this pull request Sep 2, 2022
Merged
@bouwkast bouwkast mentioned this pull request Sep 2, 2022
Merged
Base automatically changed from zach/stats/refactor-sampler to master September 7, 2022 15:19
@zacharycmontoya
zacharycmontoya force-pushed the zach/stats/discovery-update branch from 33aaf6e to 171c4fa Compare September 7, 2022 17:06
OmerRaviv
OmerRaviv approved these changes Sep 7, 2022
View reviewed changes

@OmerRaviv OmerRaviv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Didn't review, except to look for issues that could effect Live Debugger, didn't see any :)

@andrewlock

This comment has been minimized.

Sign in to view
@zacharycmontoya
- Add property IStatsAggregator.CanDropP0s (in addition to IStatsAggr…
97a56d6 
…egator.CanComputeStats)

- Remove CanComputeStats from being called internally in StatsAggregator.cs
@zacharycmontoya
- Update IDiscoveryService with the following properties and implemen…
6369e4f 
…t them in DiscoveryService:

  - string StatsEndpoint
  - bool? ClientDropP0s
@zacharycmontoya
Implement new E2E tests in StatsTests
9f9ca54
@zacharycmontoya
- Add DiscoveryService feature to StatsAggregator
fbf0084 
- In StatsTests, update tests to wait until the agent's /info endpoint is requested, to reduce test flakiness
- Add new test classes DiscoveryServiceInstanceTestCollection and DiscoveryServiceRestorerAttribute to properly test the static DiscoveryService instance
@zacharycmontoya
Refactored IStatsAggregator to only have bool? CanComputeStats, which…
11b2539 
… now returns false if we cannot drop P0 spans.
@zacharycmontoya
Very tiny refactoring
6df562f
@zacharycmontoya
Cleanup: Small code comments
528dd2a
@zacharycmontoya
PR Feedback: Change CIAgentWriter constructor to remove default value…
fa817bd 
… for IDiscoveryService parameter
@zacharycmontoya
zacharycmontoya force-pushed the zach/stats/discovery-update branch from 4a765a2 to fa817bd Compare September 7, 2022 19:29
@andrewlock

andrewlock commented Sep 7, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3152 compared to master:

  • 3 benchmarks are faster, with geometric mean 1.121
  • 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 - Faster 🎉 Same allocations ✔️

Faster 🎉 in #3152

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces‑netcoreapp3.1 1.130 524,176.64 463,678.45

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 729μs 439ns 1.7μs 0.361 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 524μs 228ns 854ns 0 0 0 2.58 KB
#3152 WriteAndFlushEnrichedTraces net472 726μs 259ns 1μs 0.359 0 0 3.18 KB
#3152 WriteAndFlushEnrichedTraces netcoreapp3.1 463μs 718ns 2.78μs 0 0 0 2.58 KB
Benchmarks.Trace.AppSecBodyBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #3152

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.AppSecBodyBenchmark.BodyExtractorSimpleBody‑netcoreapp3.1 1.119 247.55 221.24

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 200ns 0.282ns 1.05ns 0.0676 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 235ns 0.279ns 1.01ns 0.00573 0 0 424 B
master AllCycleMoreComplexBody net472 187ns 0.149ns 0.559ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 232ns 0.296ns 1.15ns 0.00534 0 0 400 B
master BodyExtractorSimpleBody net472 264ns 0.205ns 0.766ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 248ns 0.372ns 1.44ns 0.00365 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.6μs 9.12ns 34.1ns 1.21 0.0218 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.1μs 12.3ns 46ns 0.0907 0 0 6.75 KB
#3152 AllCycleSimpleBody net472 187ns 0.199ns 0.743ns 0.0675 0 0 425 B
#3152 AllCycleSimpleBody netcoreapp3.1 235ns 0.319ns 1.23ns 0.00587 0 0 424 B
#3152 AllCycleMoreComplexBody net472 188ns 0.236ns 0.915ns 0.0637 0 0 401 B
#3152 AllCycleMoreComplexBody netcoreapp3.1 232ns 0.185ns 0.693ns 0.00554 0 0 400 B
#3152 BodyExtractorSimpleBody net472 258ns 0.254ns 0.982ns 0.0574 0 0 361 B
#3152 BodyExtractorSimpleBody netcoreapp3.1 221ns 0.284ns 1.06ns 0.00365 0 0 272 B
#3152 BodyExtractorMoreComplexBody net472 14.4μs 6.75ns 25.2ns 1.2 0.0215 0 7.62 KB
#3152 BodyExtractorMoreComplexBody netcoreapp3.1 11.9μs 14ns 54.2ns 0.0899 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 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 180μs 122ns 456ns 0.27 0 0 20.57 KB
#3152 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3152 SendRequest netcoreapp3.1 181μs 111ns 430ns 0.18 0 0 20.57 KB
Benchmarks.Trace.DbCommandBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #3152

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑netcoreapp3.1 1.114 1,545.24 1,387.34

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net472 1.89μs 0.754ns 2.92ns 0.15 0.00095 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.54μs 0.828ns 3.21ns 0.0123 0 0 936 B
#3152 ExecuteNonQuery net472 1.8μs 0.879ns 3.29ns 0.15 0.000905 0 947 B
#3152 ExecuteNonQuery netcoreapp3.1 1.39μs 0.392ns 1.52ns 0.0124 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.38μs 0.829ns 3.1ns 0.183 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.52μs 1.2ns 4.64ns 0.0144 0 0 1.1 KB
master CallElasticsearchAsync net472 2.68μs 1.79ns 6.7ns 0.204 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.66μs 0.626ns 2.34ns 0.0166 0 0 1.22 KB
#3152 CallElasticsearch net472 2.52μs 1.48ns 5.72ns 0.183 0 0 1.16 KB
#3152 CallElasticsearch netcoreapp3.1 1.62μs 0.773ns 2.89ns 0.0146 0 0 1.1 KB
#3152 CallElasticsearchAsync net472 2.62μs 0.758ns 2.62ns 0.205 0 0 1.29 KB
#3152 CallElasticsearchAsync netcoreapp3.1 1.62μs 0.552ns 2.07ns 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.59μs 5.29ns 19.8ns 0.225 0 0 1.42 KB
master ExecuteAsync netcoreapp3.1 1.72μs 3.79ns 14.7ns 0.0179 0 0 1.34 KB
#3152 ExecuteAsync net472 2.52μs 6.89ns 26.7ns 0.225 0 0 1.42 KB
#3152 ExecuteAsync netcoreapp3.1 1.66μs 3.8ns 14.7ns 0.0181 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.7μs 6.5ns 23.4ns 0.438 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.57μs 6.49ns 24.3ns 0.0355 0 0 2.6 KB
#3152 SendAsync net472 5.76μs 15.7ns 60.8ns 0.439 0 0 2.77 KB
#3152 SendAsync netcoreapp3.1 3.58μs 8.05ns 31.2ns 0.0353 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.36μs 4.67ns 18.1ns 0.287 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.49μs 0.911ns 3.53ns 0.0247 0 0 1.85 KB
#3152 EnrichedLog net472 3.06μs 4.06ns 15.7ns 0.287 0 0 1.81 KB
#3152 EnrichedLog netcoreapp3.1 2.47μs 1.49ns 5.78ns 0.0246 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 151μs 219ns 847ns 0.682 0.227 0 4.65 KB
master EnrichedLog netcoreapp3.1 116μs 260ns 1.01μs 0.0585 0 0 4.49 KB
#3152 EnrichedLog net472 154μs 244ns 944ns 0.693 0.231 0 4.65 KB
#3152 EnrichedLog netcoreapp3.1 118μs 252ns 944ns 0.0585 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.56μs 11.5ns 44.7ns 0.57 0.00274 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.36μs 4.38ns 15.2ns 0.0529 0 0 3.91 KB
#3152 EnrichedLog net472 5.5μs 11.6ns 43.3ns 0.57 0.00277 0 3.59 KB
#3152 EnrichedLog netcoreapp3.1 4.45μs 5.78ns 20.8ns 0.0534 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.39μs 2.12ns 7.94ns 0.218 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.85μs 0.747ns 2.8ns 0.0174 0 0 1.32 KB
#3152 SendReceive net472 2.3μs 1.46ns 5.46ns 0.217 0 0 1.37 KB
#3152 SendReceive netcoreapp3.1 1.87μs 2.79ns 10.4ns 0.0179 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 5.07μs 1.65ns 5.94ns 0.354 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.41μs 2.07ns 8.03ns 0.0242 0 0 1.8 KB
#3152 EnrichedLog net472 5.02μs 1.93ns 7.23ns 0.352 0 0 2.23 KB
#3152 EnrichedLog netcoreapp3.1 4.3μs 1.71ns 6.64ns 0.0236 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.2μs 0.348ns 1.35ns 0.129 0 0 810 B
master StartFinishSpan netcoreapp3.1 920ns 0.231ns 0.865ns 0.0106 0 0 760 B
master StartFinishScope net472 1.41μs 0.744ns 2.88ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.09μs 0.465ns 1.74ns 0.012 0 0 880 B
#3152 StartFinishSpan net472 1.13μs 0.229ns 0.857ns 0.129 0 0 810 B
#3152 StartFinishSpan netcoreapp3.1 921ns 0.576ns 2.16ns 0.0101 0 0 760 B
#3152 StartFinishScope net472 1.38μs 0.657ns 2.37ns 0.141 0 0 891 B
#3152 StartFinishScope netcoreapp3.1 1.08μs 0.672ns 2.51ns 0.0117 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.48μs 0.375ns 1.4ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.17μs 1.47ns 5.31ns 0.0122 0 0 880 B
#3152 RunOnMethodBegin net472 1.52μs 0.349ns 1.31ns 0.141 0 0 891 B
#3152 RunOnMethodBegin netcoreapp3.1 1.19μs 0.476ns 1.72ns 0.0119 0 0 880 B

@andrewlock

andrewlock commented Sep 7, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

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

master #3152 Change
Lines 17816 / 24332 17840 / 24357
Lines % 73% 73% 0% ✔️
Branches 10500 / 14962 10509 / 14970
Branches % 70% 70% 0% ✔️
Complexity 16161 16170 9 ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3152 Change
Lines % 73% 73% 0% ✔️
Branches % 70% 70% 0% ✔️
Complexity 16161 16170 9 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Agent.NullStatsAggregator -17% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.ProbeStatuses.ProbeStatusPoller 0% ✔️ -6% ⛔ 0 ✔️
Datadog.Trace.Ci.CIVisibility 5% ✔️ 6% ✔️ 0 ✔️

View the full reports for further details:

@zacharycmontoya
zacharycmontoya merged commit 52b7d59 into master Sep 8, 2022
@zacharycmontoya
zacharycmontoya deleted the zach/stats/discovery-update branch September 8, 2022 18:01
@github-actions github-actions Bot added this to the vNext milestone Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OmerRaviv OmerRaviv OmerRaviv approved these changes
@andrewlock andrewlock andrewlock approved these changes
@pierotibou pierotibou pierotibou approved these changes

Assignees

@zacharycmontoya zacharycmontoya

Labels

area:tests unit tests, integration tests area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:enhancement Improvement to an existing feature

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

4 participants

@zacharycmontoya @andrewlock @OmerRaviv @pierotibou