feat: track ActiveRecord async queries #1351
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1219.
ActiveRecord::FutureResult
to accept a span name. Note that this is only to make it internally consistent with the existing ActiveRecord instrumentation span names; I do not believe these follow the semconv recommendations for database operations, but that can be addressed for both sync and async queries in a future version. See notes below for why this ivar was necessary.ActiveRecord::ConnectionAdapters::ConnectionPool#schedule_query
, which is the method that pushes an async task into the executor. This patch is skipped if the ConcurrentRuby instrumentation is installed.todo list:
FutureResult#execute_query
— the Rails authors seem to have just decided to forward arguments as a blob through all of the async stuff; these are re-expanded into named arguments inraw_exec_query
back in the connection handler. It's never guaranteed thatname
will remain the second argument.to_a
is called before the task is picked up, the query is executed synchronously in the parent span/context. I think it will need to be copied in as an ivar on the future result instead.User query
(existing otel instrumentation name) norUser Load
(ActiveRecord name) follow the semconv guidelines for the span name (the preferred name would beSELECT users
, but changing this is probably not the job of this PR. I'll try to at least make this internally consistent for now (edit: fixed to useUser query
)skip
) that demonstrates a false-positiveasync: true
attribute. This false positive should be fixed if possible (edit: calling this a 'wontfix' for now, as it's really a Rails bug).[ ] Decide whether or not the context propagation here is necessary, or if the concurrent-ruby instrumentation will always be installed when the Rails instrumentation is installed (see [ActiveRecord] Add Attributes for Async Queries #1219)(punted on this for now by conditionally including it)schedule [model] query
span useful at all? Seems like there is a small amount of overhead in getting the async query into the executor and then subsequently running it, so perhaps? There is also the issue that if we include it, it becomes the parent of the async query spans, which may not start before theschedule query
end_timestamp (so they are not enclosed by the parent at all).