Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tracer] Introduce async configuration for test mode to use standard writer when needed #3158

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

anmarchenko
Copy link
Member

@anmarchenko anmarchenko commented Sep 26, 2023

What does this PR do?

This PR extends Tracer's configurability:

  1. settings.tracing.test_mode.async configuration is added. When async is set to true, Datadog::Tracer is created with async writer (currently Datadog::Tracing::Writer) in test mode
  2. SHUTDOWN_TIMEOUT (determines how long to wait until worker's thread is stopped on shutdown) can be now configured via writer_options (supported both by Datadog::Tracing::Writer and Datadog::Tracing::Workers::TraceWriter)

Motivation:

Sometimes SyncWriter can cause performance issues in test mode for several reasons:

  1. It doesn't support batching and sends traces one by one
  2. It doesn't use additional threads and blocks on IO operation when using HTTP connection

An example when this behaviour is problematic is CI visibility library that uses Tracer in test mode in order to disable sampling and guarantee traces delivery.
See how this configuration is used here:

agentless_transport = Datadog::CI::TestVisibility::Transport.new(api_key: settings.api_key)

writer_options = settings.ci.writer_options
writer_options[:transport] = agentless_transport
writer_options[:shutdown_timeout] = 1000

settings.tracing.test_mode.async = true
settings.tracing.test_mode.writer_options = writer_options

Tracing's test mode is set to use async and we provide a large number as shutdown timeout for the worker to make sure that leftover traces are delivered after test suite run has ended.

Pros of this approach

  • makes test mode more flexible to be useful in more use cases
  • lets CI visibility library reuse most of the existing tracer infrastructure by substituting only the transport layer
  • do not require external libraries to use internal private class of Tracing module to achieve

Cons of this approach

  • adds coupling between Datadog::CI::TestVisibility::Transport and Datadog::Tracing::Writer because writer currently expects specific shape of Response object returned from Transport (traces_count method)
  • the interface between Writer and Transport is not really stable in Tracing and can cause issues for datadog-ci-rb gem

Alternative approaches

  1. Datadog CI Visibility library creates its own Writer, Worker and buffering logic using Datadog::Core::Workers primitives: this will require duplicating (aka copy-pasting) a lot of logic from Tracing as currently CI visibility uses Tracing public API for instrumentation and traces and spans as primitives. Currently we have no plans of changing it and developing our own set of primitives as it requires a substantial effort and all the other CI visibility libraries are built upon tracing.
  2. Datadog CI Visibility library does not use test mode: this approach would work if we make Datadog::Tracing::Sampler configurable and public which brings its own set of challenges and issues

How to test the change?
Tested in DataDog/datadog-ci-rb#33 by running sidekiq/sidekiq test suite with local versions of ddtrace + datadog-ci and agentless mode enabled

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2023

Codecov Report

Merging #3158 (d0f3808) into master (385d2fa) will increase coverage by 98.18%.
Report is 7 commits behind head on master.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #3158       +/-   ##
===========================================
+ Coverage        0   98.18%   +98.18%     
===========================================
  Files           0     1284     +1284     
  Lines           0    74105    +74105     
  Branches        0     3427     +3427     
===========================================
+ Hits            0    72757    +72757     
- Misses          0     1348     +1348     
Files Coverage Δ
lib/datadog/core/workers/polling.rb 96.55% <100.00%> (ø)
lib/datadog/tracing/component.rb 100.00% <100.00%> (ø)
lib/datadog/tracing/configuration/settings.rb 95.03% <100.00%> (ø)
lib/datadog/tracing/workers.rb 100.00% <100.00%> (ø)
lib/datadog/tracing/workers/trace_writer.rb 96.29% <100.00%> (ø)
lib/datadog/tracing/writer.rb 98.64% <100.00%> (ø)
spec/datadog/core/configuration/components_spec.rb 99.19% <100.00%> (ø)
spec/datadog/core/workers/polling_spec.rb 100.00% <100.00%> (ø)
...pec/datadog/tracing/configuration/settings_spec.rb 100.00% <100.00%> (ø)
spec/datadog/tracing/workers/trace_writer_spec.rb 94.63% <100.00%> (ø)
... and 3 more

... and 1271 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot added the core Involves Datadog core libraries label Sep 26, 2023
@anmarchenko anmarchenko marked this pull request as ready for review September 26, 2023 13:31
@anmarchenko anmarchenko requested review from a team as code owners September 26, 2023 13:31
@anmarchenko anmarchenko changed the title introduce async mode for test mode to use standard writer when needed [Tracer] Introduce async configuration for test mode to use standard writer when needed Sep 26, 2023
@anmarchenko anmarchenko merged commit 157fee6 into master Sep 27, 2023
217 checks passed
@anmarchenko anmarchenko deleted the anmarchenko/configurable_test_mode branch September 27, 2023 08:04
@github-actions github-actions bot added this to the 1.15.0 milestone Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants