Fix Rails nested partial tracing

[delner]

If you nest partials in a Rails application, one within another, e.g.:

render 'template'
  --> render 'partials/outer'
    --> render 'partials/inner'

You will receive an error that looks something like root span closed but has 1 unfinished spans. Those traces affected won't be written or propagated to your account. The bug is confirmed to affect Rails 3.2.

This was occurring because when partials were nested in one another, a certain contextual state object (whose purpose was to describe one partial) was being shared, then overwritten by its nested partial children. Overwriting this state effectively corrupted at least one span within the trace, thus causing it to not complete correctly.

This pull request implements a hash of contexts, which are each tied to an individual span by ID. This allows the existing code to find and access the context appropriate to it.

The strategy for context management for partials could benefit a great deal from a refactor. But in the mean time, this change should serve as an effective stopgap measure to address the current bug.

[p-lambert]

More a matter of style, but I've found these methods a bit convoluted/hard to follow.

I think the code below is much easier to read:

@tracing_context[span.id][:template_name] = ...

Why perform like 5 method calls to achieve roughly the same thing as the line above?

In case you're really providing some safe-guards, exposing just a subset of hash operations, or ensuring some kind of integrity for the values, I would instead create a class representing that, but I do think that would be also overkill.

[delner]

I don't think you can do @tracing_context[span.id][:template_name], I think you have to deference the span from the current call context, then pass that through. e.g.

span = Datadog.configuration[:rails][:tracer].call_context.current_span
@tracing_context[span.id][:template_name] = ...

The first line of which you'd have to do in each function you reference the context (which I think ends up being twice here.) Yeah, I can see why 5 small functions might be not as quick to read themselves, but I think it makes the function they're used in more expressive and easier to read. Also it'd be slightly better in the sense of Law of Demeter, if you care about that sort of thing.

To be fair, I think the whole implementation could use a refactor to some kind of better strategy at some point, which might change things anyways.

[p-lambert]

Well, in that case I would have a helper method current_span_id then:

@tracing_context[current_span_id][:template_name]

I do care about the about the amount of coupling between components (Law of Demeter), but I honestly think that can't be solved by splitting a simple operation among a bunch of method calls.

Anyway, not really a blocking issue, so I'm fine with it.

@delner

[delner]

@p-lambert I think that's pretty reasonable. I'll give that a shot.

[p-lambert]

I think not having the unless conditional here would be better.

• We won't have a leak because we delete the entry anyways;
• If current_span_id is nil with the current implementation, you'll crash because you're doing hash operations in the following lines without the conditional;

So my understanding is: you either remove it or replicate in all lines referencing tracing_contexts

[delner]

Should be addressed now.