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

Rake:Require instrumented task list #2174

Merged
merged 1 commit into from
Jul 23, 2022
Merged

Rake:Require instrumented task list #2174

merged 1 commit into from
Jul 23, 2022

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Jul 22, 2022

Release notes

Users of the Rake instrumentation will have to explicitly list the tasks that they want instrumented. This was done to prevent memory bloat issues that can arise from instrumenting long-running Rake tasks.

From today's configuration:

Datadog.configure do |c|
  c.tracing.instrument :rake
end

The list of desired Rake tests to instrument should be added:

Datadog.configure do |c|
  c.tracing.instrument :rake, tasks: ['task1', 'task2', ...]
end

Not providing this list will effectively instrument no Rake tasks.

What does this PR do?

This PR requires the Rake instrumentation to receive an explicit list of task to be instrumented. This list can be a list of String task names, or other values that can be stringified (e.g. it's common to declare Rake tasks with Symbol names).

Motivation

Long-running Rake tasks, those measured in days, can accumulate a very large active trace in memory. This normally won't be flushed until the application terminates.

Such traces can cause catastrophic memory accumulation, possibly triggering an out-of-memory process shutdown.

Examples of such cases can be found in #2045.

Additional Notes

I chose to no require users to have all their Rake loaded before invoking Datadog.configure: our current documentation suggests that users configure ddtrace before Rake task definition, so I chose to maintain compatibility and such flexibility.
Also, the implementation is not very complicated: there's a small performance overhead on non-instrumented tasks, but it boils down to a Set#include? call.
The dynamic implementation also allows for dynamic reconfiguration of the tasks: configuration list. The eager approach will either "patch" the Rake tasks and never be able to "unpatch", or it will implement the dynamic approach in this PR as well as selective patching.

Testing for the unrelated when tracing is disabled case was a bit misleading around tracer #shutdown! calls: it was asserting on a Datadog::Tracing.trace instance, but another instance of the tracer was used during testing. This PR fixes that.

How to test the change?

Unit test coverage around Rake tasks is already pretty good. I added the relevant cases introduced in this PR.

@marcotc marcotc added the bug Involves a bug label Jul 22, 2022
@marcotc marcotc requested a review from a team July 22, 2022 00:10
@marcotc marcotc self-assigned this Jul 22, 2022
Copy link
Contributor

@TonyCTHsu TonyCTHsu left a comment

Choose a reason for hiding this comment

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

Looks good!

@marcotc marcotc merged commit a335fb1 into master Jul 23, 2022
@marcotc marcotc deleted the rake-explicit branch July 23, 2022 00:10
@github-actions github-actions bot added this to the 1.3.0 milestone Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants