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

Add Datadog::Worker and extensions #973

Merged
merged 4 commits into from
Mar 30, 2020
Merged

Conversation

delner
Copy link
Contributor

@delner delner commented Mar 10, 2020

Extracted from #879

This pull request adds the Datadog::Worker base class and its extension modules. It is intended to act as the foundation for background processes that run within the tracing library. Examples of these would be "writing traces to the agent" and "flushing runtime metrics to Statsd".

At its core, there is the Datadog::Worker class which itself defines a simple task, and a perform function.

Each background process would extend this base class, and define its work behavior within. e.g.

class TraceWriter < Datadog::Worker
  def perform(traces)
    # Write traces to agent
  end
end

Because each worker might need to behave differently (e.g. polling, queuing, threading etc) these behaviors are provided through modules Polling, Async::Thread, IntervalLoop, Queue which can then be composed into the worker. Each of these layers wrap the perform function with additional behavior, and provide new functions to the worker. e.g.

class AsyncTraceWriter < TraceWriter
  include Workers::Queue
  include Workers::Polling
end

class RuntimeMetrics < Datadog::Worker
  include Workers::Polling

  def perform
    # Flush runtime metrics
  end
end

As described in #879, some of the benefits of implementing this include:

  • Allows us to eliminate the SyncWriter by having one common definition for trace flushing behavior, then having AsyncTraceWriter decorated upon that.
  • AsyncTraceWriter can optionally automatically switch to synchronous writing when forked (addresses the need for SyncWriter in integrations like Resque.)
  • Breaks the worker behaviors into smaller, composable behaviors (e.g. Async, Queue, IntervalLoop, etc) which should be easier to test and re-use, which will...
  • ...allow us to extract and introduce a runtime metrics worker. These changes make building & maintaining the runtime metrics worker easier because it shares many of the same basic worker components

The Datadog::Worker and its other components will be used in another set of PRs which will introduce the TraceWriter and RuntimeMetrics workers.

@delner delner added core Involves Datadog core libraries feature Involves a product feature labels Mar 10, 2020
@delner delner added this to the 0.34.0 milestone Mar 10, 2020
@delner delner requested review from marcotc, brettlangdon and a team March 10, 2020 18:13
@delner delner self-assigned this Mar 10, 2020
@delner delner force-pushed the feature/add_workers_and_extensions branch from a0b11e2 to 5432b63 Compare March 10, 2020 18:29
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Great stuff, very well flushed out implementation. 👍 🎉

lib/ddtrace/workers/async.rb Outdated Show resolved Hide resolved
lib/ddtrace/workers/async.rb Outdated Show resolved Hide resolved
lib/ddtrace/workers/loop.rb Outdated Show resolved Hide resolved
end

def buffer
@buffer ||= []
Copy link
Member

Choose a reason for hiding this comment

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

Is it ok to use a simple array here, as it is not thread-safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose it depends on your worker implementation, and how (also if) it uses threads. The buffer itself should be made thread safe if you require it; we do this by overriding the buffer for the TraceWriter. This is just a default implementation.

spec/ddtrace/workers/async_spec.rb Outdated Show resolved Hide resolved
Comment on lines +19 to +20
# Stub conditional wait so tests run faster
before { allow(worker.send(:shutdown)).to receive(:wait) }
Copy link
Member

Choose a reason for hiding this comment

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

Awesome quality of life improvement with this. 💯

spec/support/synchronization_helpers.rb Outdated Show resolved Hide resolved
@delner delner force-pushed the feature/add_workers_and_extensions branch from 5432b63 to 0abe5d9 Compare March 30, 2020 15:08
@delner delner force-pushed the feature/add_workers_and_extensions branch from 0abe5d9 to 9f7c7be Compare March 30, 2020 15:12
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Looking forward to seeing these changes in the wild! ⛵️🌊

@delner delner merged commit 64c5f25 into master Mar 30, 2020
@delner delner deleted the feature/add_workers_and_extensions branch March 30, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries feature Involves a product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants