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

Allow joining executions to jobs scoped by state #886

Conversation

segiddins
Copy link
Contributor

To do this, we need arel to be able to track the current table name for jobs (otherwise we get ambiguous column errors), so we construct arel nodes instead of using raw sql strings

This allows me to do something like:

Filter.new({}).filtered_query(state:)
.joins(:executions).then do |rel|
      rel.pluck(
        *rel.group_values,
        Arel::Nodes::Max.new(
          [Arel::Nodes.build_quoted(now, rel.arel_table[:created_at]) -
            rel.arel_table.coalesce(*rel.send(:arel_columns, %w[executions_good_jobs.performed_at executions_good_jobs.finished_at executions_good_jobs.scheduled_at executions_good_jobs.created_at]))]
        )
      )
        .to_h { |*a, v| [a, v] }
    end

To do this, we need arel to be able to track the current table name for jobs (otherwise we get ambiguous column errors), so we construct arel nodes instead of using raw sql strings
Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

This is great! Thank you! I just pushed up some fixes for some naming collisions and I'll merge.

@segiddins
Copy link
Contributor Author

Ah nice I was just about to fix that :)

In case it interests you, this is going towards rubygems/rubygems.org#3582, which adds some statsd instrumentation that is aimed at a more job-centric latency than an individual execution one (basically, we had a few days of retries pile up when we switched one job over to GoodJob and our dashboard just showed the queue depth, but not the fast that some of them shouldve been run days ago)

@bensheldon
Copy link
Owner

In case it interests you, this is going towards rubygems/rubygems.org#3582, which adds some statsd instrumentation that is aimed at a more job-centric latency than an individual execution one

Yikes! Sorry that the dashboard gave a false sense that everything was fine 😬 I want to improve those charts in #438 (I have some bad SQL in #888). For queue latency I'm thinking the query needs to be something like COALESCE(performed_at, NOW()) - COALESCE(scheduled_at, created_at) because that will allow jobs that haven't begun to run at all to accumulate latency in the calculation by using NOW().

@bensheldon bensheldon merged commit e01fa79 into bensheldon:main Mar 14, 2023
@segiddins
Copy link
Contributor Author

Ah no it wasn't your dashboard :) it was the one I built off of our statsd metrics, and I knew with was wrong

@bensheldon
Copy link
Owner

Ah no it wasn't your dashboard :)

Well, my Dashboard also has that problem 😭

This and #885 was released in https://github.com/bensheldon/good_job/releases/tag/v3.14.1

segiddins added a commit to rubygems/rubygems.org that referenced this pull request Mar 14, 2023
segiddins added a commit to rubygems/rubygems.org that referenced this pull request Mar 14, 2023
@ollym
Copy link

ollym commented Mar 15, 2023

@bensheldon @segiddins hang about, can you make this change also for GoodJob::Execution seems inconsistent to do it for Job and not Execution.

I'm asking as I need to update my patch here:
#876 (comment)

And would be far easier if i can just override the params_execution_count and params_job_class methods in both classes.

@bensheldon
Copy link
Owner

@ollym oops, sorry about that! I'll make that change today.

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