Skip to content

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

Merged
anmarchenko merged 7 commits into
masterfrom
anmarchenko/configurable_test_mode
Sep 27, 2023
Merged

[Tracer] Introduce async configuration for test mode to use standard writer when needed#3158
anmarchenko merged 7 commits into
masterfrom
anmarchenko/configurable_test_mode

Conversation

@anmarchenko

@anmarchenko anmarchenko commented Sep 26, 2023

Copy link
Copy Markdown
Member

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

codecov-commenter commented Sep 26, 2023

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.18%. Comparing base (385d2fa) to head (d0f3808).
⚠️ Report is 5213 commits behind head on master.

Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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