Putting rich objects into the payload might be against the spirit of the logging system.

[ioquatix]

dd-trace-rb/lib/datadog/tracing/contrib/active_support/notifications/subscription.rb

Line 81 in f3c7f96

payload[:datadog_span] = span

In our logging system, we don't expect rich objects in the payload - we expect things that can serialize to JSON.

Maybe I'm wrong, but I think putting a datadog span into the payload is a poor design as tracing should not affect the implementation of the system IMHO. At best, this seems a bit leaky.

Is there some way we can refactor this code to avoid plumbing datadog spans into the log payload?

[marcotc]

Hey @ioquatix, thank you for reporting this issue.

We can do away with storing the span object as a payload value with a bit of creative thinking.

In our logging system, we don't expect rich objects in the payload - we expect things that can serialize to JSON.

I'm wondering: is the contract for the payload Hash, from ActiveSupport::Notifications::Instrumenter#instrument, that it should be serializable?

I ask because ActiveSupport themselves store complex objects into it: https://github.com/rails/rails/blob/a8871e6829e55dd7943bc914971c0f07271372e3/activesupport/lib/active_support/notifications/instrumenter.rb#L61

[ioquatix]

You are not wrong - in fact, they store the Rack request and response objects too.

So, I'm probably wrong about not storing rich objects in the payload.

However, it was unexpected and broke our logging.

When I first looked at this code, I was a bit surprised. But now after sleeping on it, I suppose I should be more specific/explicit about what fields we are logging.

If you want to close this issue as "works as intended" that's totally fine.

However, I also wonder if using the log notification system is the best way to instrument ActiveRecord. It's definitely tricky since you are not on the stack with a begin ... start trace ... ensure ... end trace ... style logic.

Did the Rails team provide any guidance on the most appropriate way to add instrumentation/tracing?

[delner]

Did the Rails team provide any guidance on the most appropriate way to add instrumentation/tracing?

@ioquatix No, but to be fair, the code is relatively old and I didn't know who to ask. If you have any suggestions on where to start that, that'd be helpful.

If there's a reliable way to measure Rails activities such as this in a first-class way, we'd happily use that. I thought that's what ActiveSupport::Notifications was meant to be.

Also, the other small wrinkle is we aim to support old versions of Rails too (ideally down to 4.x.) I don't see a problem rolling instrumentation per major version of Rails, if necessary, but the more dependable/public interfaces we can utilize, the better.

[ioquatix]

I'll do a little digging and see what I can turn up.