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

Add cursor next iteration metric #340

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Add cursor next iteration metric #340

merged 1 commit into from
Aug 29, 2023

Conversation

etravassos
Copy link
Contributor

@etravassos etravassos commented Feb 15, 2023

This PR introduces a new instrumentation that captures the time to execute a query to fetch records for an Enumerator next batch call.

Note that the existing build_enumerator.iteration instrumentation does not capture the time of the query execution given that the enumerator lazy loads the data and the actual query is only executed in the next batch call.

Motivation

  • While investigating job execution times we needed to drill down and separate the time a job takes to retrieve data vs the time it takes to process the data.

Usage

  • the new instrumentation can be used as follows
ActiveSupport::Notifications.subscribe("cursor.iteration") do |*args|
  ...
end

@etravassos etravassos force-pushed the eo/build_enum_metric branch 6 times, most recently from 2e0a952 to 0e791d6 Compare February 17, 2023 17:21
@etravassos etravassos marked this pull request as ready for review February 17, 2023 19:51
@etravassos etravassos force-pushed the eo/build_enum_metric branch 5 times, most recently from 5d8bb8a to b80895b Compare February 23, 2023 17:05
Copy link
Member

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's at least the ActiveRecordBatchEnumerator that would be interesting to instrument as well. But yes it's a good idea!

@@ -39,6 +39,12 @@ def size

private

def instrument_next_batch(cursor)
ActiveSupport::Notifications.instrument("cursor.iteration") do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ActiveSupport::Notifications.instrument("cursor.iteration") do
ActiveSupport::Notifications.instrument("active_record_cursor.iteration") do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey 👋 @etiennebarrie @Mangara
Pushed the tweak if you could take another look.

@etravassos etravassos merged commit 23c69dc into main Aug 29, 2023
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems August 29, 2023 20:48 Inactive
@sambostock sambostock deleted the eo/build_enum_metric branch February 6, 2024 15:22
@etiennebarrie etiennebarrie restored the eo/build_enum_metric branch November 27, 2024 15:03
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.

3 participants