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.

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

feat: track ActiveRecord async queries #1351

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zvkemp
Copy link

@zvkemp zvkemp commented Jan 17, 2025

Fixes #1219.

[Description WIP]

still todo:

  • test with async aggregation queries (e.g. #async_count)
  • test with multiple concurrent queries (ended up not using any stacks or additional context, so there are unlikely to be issues here).
  • I don't love how the span name is derived in 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 in raw_exec_query back in the connection handler. It's never guaranteed that name will remain the second argument.
    • more on this: unfortunately we also can't guarantee that the query is executed in an otel context that can be propagated from the scheduler method — i.e. we can set the desired span name into the context, which works if the async executor enqueues the task or executes it inline, because we can copy that context into the task. However, if 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.
  • Neither User query (existing otel instrumentation name) nor User Load (ActiveRecord name) follow the semconv guidelines for the span name (the preferred name would be SELECT 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 use User query)
  • There is one test (the one that calls skip) that demonstrates a false-positive async: 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)
  • is the schedule 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 the schedule query end_timestamp (so they are not enclosed by the parent at all).

@zvkemp zvkemp force-pushed the active-record-async branch 2 times, most recently from d4184ae to a693b55 Compare January 17, 2025 20:09
@zvkemp zvkemp force-pushed the active-record-async branch from a693b55 to 8b1ccd9 Compare January 17, 2025 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ActiveRecord] Add Attributes for Async Queries
1 participant