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.

Sign up for GitHub

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

Fix ActionView tracing contexts when span limit is reached #447

Merged
merged 3 commits into from
Jun 11, 2018
+69 −115
Merged

Fix ActionView tracing contexts when span limit is reached #447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7db980
Refactored: Rails PartialRenderer tracing.
delner Jun 7, 2018
e9b9809
Refactored: Rails TemplateRenderer tracing.
delner Jun 7, 2018
eafb1d7
Changed: Move some strings into constants.
delner Jun 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 0 additions & 65 deletions lib/ddtrace/contrib/rails/action_view.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,71 +13,6 @@ def self.instrument
Datadog::RailsRendererPatcher.patch_renderer
end
end

def self.start_render_template(payload)
# retrieve the tracing context
tracing_context = payload.fetch(:tracing_context)

# create a new Span and add it to the tracing context
tracer = Datadog.configuration[:rails][:tracer]
span = tracer.trace('rails.render_template', span_type: Datadog::Ext::HTTP::TEMPLATE)
tracing_context[:dd_rails_template_span] = span
rescue StandardError => e
Datadog::Tracer.log.debug(e.message)
end

def self.finish_render_template(payload)
# retrieve the tracing context and the latest active span
tracing_context = payload.fetch(:tracing_context)
span = tracing_context[:dd_rails_template_span]
return if !span || span.finished?

# finish the tracing and update the execution time
begin
template_name = tracing_context[:template_name]
layout = tracing_context[:layout]
exception = tracing_context[:exception]

span.set_tag('rails.template_name', template_name) if template_name
span.set_tag('rails.layout', layout) if layout
span.set_error(exception) if exception
ensure
span.finish()
end
rescue StandardError => e
Datadog::Tracer.log.debug(e.message)
end

def self.start_render_partial(payload)
# retrieve the tracing context
tracing_context = payload.fetch(:tracing_context)

tracer = Datadog.configuration[:rails][:tracer]
span = tracer.trace('rails.render_partial', span_type: Datadog::Ext::HTTP::TEMPLATE)
tracing_context[:dd_rails_partial_span] = span
rescue StandardError => e
Datadog::Tracer.log.debug(e.message)
end

def self.finish_render_partial(payload)
# retrieve the tracing context and the latest active span
tracing_context = payload.fetch(:tracing_context)
span = tracing_context[:dd_rails_partial_span]
return if !span || span.finished?

# finish the tracing and update the execution time
begin
template_name = tracing_context[:template_name]
exception = tracing_context[:exception]

span.set_tag('rails.template_name', template_name) if template_name
span.set_error(exception) if exception
ensure
span.finish()
end
rescue StandardError => e
Datadog::Tracer.log.debug(e.message)
end
end
end
end
Expand Down
111 changes: 68 additions & 43 deletions lib/ddtrace/contrib/rails/core_extensions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ module Datadog
module RailsRendererPatcher
include Datadog::Patcher

SPAN_NAME_RENDER_PARTIAL = 'rails.render_partial'.freeze
SPAN_NAME_RENDER_TEMPLATE = 'rails.render_template'.freeze
TAG_LAYOUT = 'rails.layout'.freeze
TAG_TEMPLATE_NAME = 'rails.template_name'.freeze

module_function

def patch_renderer
Expand All @@ -24,25 +29,22 @@ def patch_renderer
end

def patch_template_renderer(klass)
# rubocop:disable Metrics/BlockLength
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider increasing this limit? So that adding exceptions will not be necessary?

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'm okay with that. To me, this metric seems a bit arbitrary/silly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I tried this, but it creates a bunch of other Rubocop problems in other files unrelated to this PR. I don't think we should do this right now; we have a task in the pipeline to cover Rubocop refactoring, so we should do this then.

do_once(:patch_template_renderer) do
klass.class_eval do
def render_with_datadog(*args, &block)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can methods defined here be extracted to a module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, they could. I didn't want to do that here because doing so would depend on prepend, we'd end up with version differences between 1.9.3 and 2.0.0+, and then everything would get more complicated, which would increase the risk of creating more bugs. We had just experienced exactly this when we refactored the ActionController patch into a module.

I would like to do this, but we should do it in a separate PR, because its more important we have a stable, verified fix to ship first. Then when that's out, we can clean up the implementation to make things look cleaner.

Copy link
Contributor

@pawelchcki pawelchcki Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Would moving only def render_with_datadog to a separate module still require 'prepend'?
Ok lets handle that in separate PR.

# create a tracing context and start the rendering span
# NOTE: Rails < 3.1 compatibility: preserve the tracing
# context when a partial is rendered
@tracing_context ||= {}
if @tracing_context.empty?
Datadog::Contrib::Rails::ActionView.start_render_template(tracing_context: @tracing_context)
# NOTE: This check exists purely for Rails 3.0 compatibility.
# The 'if' part can be removed when support for Rails 3.0 is removed.
if active_datadog_span
render_without_datadog(*args, &block)
else
datadog_tracer.trace(
Datadog::RailsRendererPatcher::SPAN_NAME_RENDER_TEMPLATE,
span_type: Datadog::Ext::HTTP::TEMPLATE
) do |span|
with_datadog_span(span) { render_without_datadog(*args, &block) }
end
end

render_without_datadog(*args, &block)
rescue Exception => e
# attach the exception to the tracing context if any
@tracing_context[:exception] = e
raise e
ensure
# ensure that the template `Span` is finished even during exceptions
Datadog::Contrib::Rails::ActionView.finish_render_template(tracing_context: @tracing_context)
end

def render_template_with_datadog(*args)
Expand All @@ -60,8 +62,19 @@ def render_template_with_datadog(*args)
else
layout_name.try(:[], 'virtual_path')
end
@tracing_context[:template_name] = template_name
@tracing_context[:layout] = layout
if template_name
active_datadog_span.set_tag(
Datadog::RailsRendererPatcher::TAG_TEMPLATE_NAME,
template_name
)
end

if layout
active_datadog_span.set_tag(
Datadog::RailsRendererPatcher::TAG_LAYOUT,
layout
)
end
rescue StandardError => e
Datadog::Tracer.log.debug(e.message)
end
Expand All @@ -70,6 +83,21 @@ def render_template_with_datadog(*args)
render_template_without_datadog(*args)
end

private

attr_accessor :active_datadog_span

def datadog_tracer
Datadog.configuration[:rails][:tracer]
end

def with_datadog_span(span)
self.active_datadog_span = span
yield
ensure
self.active_datadog_span = nil
end

# method aliasing to patch the class
alias_method :render_without_datadog, :render
alias_method :render, :render_with_datadog
Expand All @@ -90,30 +118,23 @@ def patch_partial_renderer(klass)
do_once(:patch_partial_renderer) do
klass.class_eval do
Copy link
Contributor

Choose a reason for hiding this comment

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

Could methods defined here be extracted to a module as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same feedback as above.

def render_with_datadog(*args, &block)
# Create a tracing context and start the rendering span
tracing_context = {}
Datadog::Contrib::Rails::ActionView.start_render_partial(tracing_context: tracing_context)
tracing_contexts[current_span_id] = tracing_context

render_without_datadog(*args)
rescue Exception => e
# attach the exception to the tracing context if any
tracing_contexts[current_span_id][:exception] = e
raise e
ensure
# Ensure that the template `Span` is finished even during exceptions
# Remove the existing tracing context (to avoid leaks)
tracing_contexts.delete(current_span_id)

# Then finish the span associated with the context
Datadog::Contrib::Rails::ActionView.finish_render_partial(tracing_context: tracing_context)
datadog_tracer.trace(
Datadog::RailsRendererPatcher::SPAN_NAME_RENDER_PARTIAL,
span_type: Datadog::Ext::HTTP::TEMPLATE
) do |span|
with_datadog_span(span) { render_without_datadog(*args) }
end
end

def render_partial_with_datadog(*args)
begin
# update the tracing context with computed values before the rendering
template_name = Datadog::Contrib::Rails::Utils.normalize_template_name(@template.try('identifier'))
tracing_contexts[current_span_id][:template_name] = template_name
if template_name
active_datadog_span.set_tag(
Datadog::RailsRendererPatcher::TAG_TEMPLATE_NAME,
template_name
)
end
rescue StandardError => e
Datadog::Tracer.log.debug(e.message)
end
Expand All @@ -122,15 +143,19 @@ def render_partial_with_datadog(*args)
render_partial_without_datadog(*args)
end

# Table of tracing contexts, one per partial/span, keyed by span_id
# because there will be multiple concurrent contexts, depending on how
# many partials are nested within one another.
def tracing_contexts
@tracing_contexts ||= {}
private

attr_accessor :active_datadog_span

def datadog_tracer
Datadog.configuration[:rails][:tracer]
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

end

def current_span_id
Datadog.configuration[:rails][:tracer].call_context.current_span.span_id
def with_datadog_span(span)
self.active_datadog_span = span
yield
ensure
self.active_datadog_span = nil
end

# method aliasing to patch the class
Expand Down
8 changes: 1 addition & 7 deletions test/contrib/rails/errors_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,6 @@ class TracingControllerTest < ActionController::TestCase
'Missing partial tracing/ouch.html'
end

template_error_type = if Rails.version >= '3.2.22.5'
'ActionView::Template::Error'
else
'ActionView::MissingTemplate'
end

spans = @tracer.writer.spans()
assert_equal(spans.length, 3)
span_request, span_partial, span_template = spans
Expand Down Expand Up @@ -131,7 +125,7 @@ class TracingControllerTest < ActionController::TestCase
assert_equal(span_template.get_tag('rails.template_name'), 'tracing/missing_partial.html.erb')
assert_equal(span_template.get_tag('rails.layout'), 'layouts/application')
assert_includes(span_template.get_tag('error.msg'), error_msg)
assert_equal(span_template.get_tag('error.type'), template_error_type)
assert_equal(span_template.get_tag('error.type'), 'ActionView::Template::Error')
end

test 'error in the template must be traced' do
Expand Down