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

Lazily activate integrations #1037

Merged
merged 2 commits into from
May 13, 2020
Merged

Lazily activate integrations #1037

merged 2 commits into from
May 13, 2020

Conversation

delner
Copy link
Contributor

@delner delner commented May 11, 2020

Today, in a configuration block...

Datadog.configure do |c|
  c.use :faraday
end

c.use configures and patches the integration immediately. This has the unwanted side effect of causing trace components being initialized with default settings before the configuration block is completed, because patching integrations often calls Datadog.logger which is a trace component.

In this pull request, c.use adds the integration to a list of pending integrations, which are each patched after the configuration block completes and the trace components are initialized. This prevents trace components from being re-initialized multiple times during setup.

@delner delner added bug Involves a bug integrations Involves tracing integrations labels May 11, 2020
@delner delner requested a review from marcotc May 11, 2020 00:26
@delner delner self-assigned this May 11, 2020
@delner
Copy link
Contributor Author

delner commented May 11, 2020

Rails minitests don't like these changes for some reason, still haven't figured it out. Once CI is green this will be ready for review.

@delner delner force-pushed the fix/lazy_activate_integrations branch from 5a297e3 to 7327e16 Compare May 11, 2020 00:39
@delner delner changed the base branch from master to fix/configure_eager_loading_lazy_vars May 11, 2020 15:01
@delner delner changed the base branch from fix/configure_eager_loading_lazy_vars to master May 11, 2020 15:06
@delner delner force-pushed the fix/lazy_activate_integrations branch from 7327e16 to 81e1aa3 Compare May 11, 2020 15:18
@delner delner force-pushed the fix/lazy_activate_integrations branch from 81e1aa3 to 17e1e38 Compare May 11, 2020 17:52
@delner delner marked this pull request as ready for review May 11, 2020 18:17
@delner delner requested a review from a team May 11, 2020 18:17
@delner
Copy link
Contributor Author

delner commented May 11, 2020

Figured out why the Rails tests were failing.

When Rails is configured with Datadog.configure { |c| c.use :rails }, its patch behavior is to hook into before_initialize and after_initialize callbacks in the Rails application lifecycle. These callbacks are executed later to complete patching and configuration for Rails. It does this because there are some configuration settings like the application name and Rack application stack that are not available until later in the Rails startup process.

Prior to this PR, the after_initialize hook was updating Datadog configuration by calling Datadog.configuration.use :action_pack and other similar commands for the other Rails components. This was okay because patching ran immediately on c.use, so those components were patched properly.

In this PR after we switched to lazy patching, c.use no longer applies patches, it 'queues' integrations to be patched. Since patches are only applied when Datadog.configure completes, these patches were remaining 'queued.' The Datadog.configuration.use :action_pack command in after_initialize was thus not applying the patch to ActionPack or the other components. To fix this, we have to Datadog.configure.

This is not the only reason we have to Datadog.configure: since the default service name can only be sourced from Rails after its loaded, it must update c.service in after_initialize as well. In order for the tracer and other components to appropriately use this updated service name, they must be rebuilt, and Datadog.configure is the function that triggers that behavior.

The unfortunate side effect is that for Rails applications, the tracer components are effectively initialized twice; once after Datadog.configure in initializers/datadog.rb and a second time after the Rails application finishes loading in after_initialize. This should have no functional impact on Rails applications, and the cost of rebuilding is not very expensive, but it is nonetheless undesirable and (thus far seemingly) unavoidable.

Despite this, I think we do have a reasonable solution in place, and while it isn't perfect, it stands to grant benefits that justify any imperfections we've seen and considered thus far.

@delner delner added this to In review in Active work May 11, 2020
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.

👍

@delner delner merged commit d09b203 into master May 13, 2020
Active work automation moved this from In review to Merged & awaiting release May 13, 2020
@delner delner deleted the fix/lazy_activate_integrations branch May 13, 2020 14:09
@marcotc marcotc added this to the 0.36.0 milestone May 27, 2020
@delner delner linked an issue May 28, 2020 that may be closed by this pull request
@saarons
Copy link

saarons commented Jun 12, 2020

@delner Something might be strange in my configuration but this change has the unfortunate side-effect of creating 2 instances of Datadog::Tracer.

ObjectSpace.each_object(Datadog::Tracer).count #=> 2

One of these is accessible by calling Datadog.tracer in the Rails application, but the other is seemingly not as easy to access. The issue is that Rails is instrumented with the tracer I can't access directly, so calling things like Datadog.tracer.active_correlation just give me empty spans when called inside my app code.

@saarons
Copy link

saarons commented Jun 12, 2020

@delner Ignore me, I see you're already on it in #1064

@delner delner moved this from Merged & awaiting release to Released in Active work Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug integrations Involves tracing integrations
Projects
Active work
  
Released
Development

Successfully merging this pull request may close these issues.

Add an option to error if an internal integration is the root span
3 participants