Fix ActionView tracing contexts when span limit is reached #447
Conversation
delner
added
bug
There was a problem hiding this comment.
Thanks for the PR @delner. The new approach looks much cleaner compared to old.
Just left a few remarks about possible some small resource use gains and about extracting some of the code to a module to avoid having to add exceptions to rubocop.
| @@ -24,25 +24,19 @@ def patch_renderer | |||
| end | |||
|
|
|||
| def patch_template_renderer(klass) | |||
| # rubocop:disable Metrics/BlockLength | |||
There was a problem hiding this comment.
Should we consider increasing this limit? So that adding exceptions will not be necessary?
There was a problem hiding this comment.
I'm okay with that. To me, this metric seems a bit arbitrary/silly.
There was a problem hiding this comment.
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.
| @@ -24,25 +24,19 @@ def patch_renderer | |||
| end | |||
|
|
|||
| def patch_template_renderer(klass) | |||
| # rubocop:disable Metrics/BlockLength | |||
| do_once(:patch_template_renderer) do | |||
| klass.class_eval do | |||
| def render_with_datadog(*args, &block) | |||
There was a problem hiding this comment.
Can methods defined here be extracted to a module?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Would moving only def render_with_datadog to a separate module still require 'prepend'?
Ok lets handle that in separate PR.
| @@ -90,30 +99,15 @@ def patch_partial_renderer(klass) | |||
| do_once(:patch_partial_renderer) do | |||
| klass.class_eval do | |||
There was a problem hiding this comment.
Could methods defined here be extracted to a module as well ?
There was a problem hiding this comment.
Same feedback as above.
| attr_accessor :active_datadog_span | ||
|
|
||
| def datadog_tracer | ||
| Datadog.configuration[:rails][:tracer] |
| template_name = Datadog::Contrib::Rails::Utils.normalize_template_name(@template.try('identifier')) | ||
| tracing_contexts[current_span_id][:template_name] = template_name | ||
| active_datadog_span.set_tag('rails.template_name', template_name) if template_name |
There was a problem hiding this comment.
'rails.template_name'.freeze this will avoid allocating (and then GCing) the same string multiple times within single request.
There was a problem hiding this comment.
You know, I tried doing exactly this, after I saw your PR. It's actually not possible to add a constant to the module we're patching here, because it raises a dynamic constant error. The only way I could think around this is either 1) refactoring this into a module (preferable, but must defer per feedback above), or 2) defining the constant in another namespace then referencing that.
The 2nd option I could do now, it just won't be as clean.
| @@ -60,8 +54,8 @@ def render_template_with_datadog(*args) | |||
| else | |||
| layout_name.try(:[], 'virtual_path') | |||
| end | |||
| @tracing_context[:template_name] = template_name | |||
| @tracing_context[:layout] = layout | |||
| active_datadog_span.set_tag('rails.template_name', template_name) if template_name | |||
There was a problem hiding this comment.
freezeing this string might also help a bit with memory use.
| @tracing_context[:template_name] = template_name | ||
| @tracing_context[:layout] = layout | ||
| active_datadog_span.set_tag('rails.template_name', template_name) if template_name | ||
| active_datadog_span.set_tag('rails.layout', layout) if layout |
There was a problem hiding this comment.
freezeing this string might also help a bit with memory use.
|
@pawelchcki Addressed feeback. Let me know what you think. |
As revealed by #445, when the hard-cap span limit is reached for a trace (~100K), the Rails renderer tracing starts mishandling spans. This is because it was relying on
tracer.call_context.current_spanwhich returns the last span in the context when the context fills, as opposed to the most recently instantiated span.Although addressing the behavior of
tracer.call_context.current_spanand its equivalenttracer.active_spanremains an issue at large, we can avoid this altogether in the Rails code.This pull request removes "tracing context" management and context-specific code from the Rails patch altogether, and opts for a much simpler
tracer.traceapproach instead. This should resolve #445 by simply avoiding the code path in question altogether.