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

Measure contact rollups job #29073

Merged
merged 15 commits into from Jun 17, 2019
Merged

Measure contact rollups job #29073

merged 15 commits into from Jun 17, 2019

Conversation

hacodeorg
Copy link
Contributor

@hacodeorg hacodeorg commented Jun 11, 2019

PLC-258

What

  • Time major steps in Contact Rollups job. Propagate results directly to cron-daily Slack channel (example).
  • Raise HoneyBadger error when validation fails so the job cannot fail silently.

Why

  • Contact Rollups is a long running and resource intensive job (~3h, cannot run directly on production db) and has no test. It sometime failed for more than 1 week without notice. It has been a major tech debt and we will soon have to re-architect it.
  • The first few steps are
    • Time important steps in the process to identify bottlenecks.
    • Collect important metrics such as # of new/updated rows to identify which steps are still valuable to keep.

How tested

  • Test LogCollector bundle exec ruby lib/test/cdo/test_log_collector.rb
  • Test run in test server
    • Check out (cherry-pick) files in feature branch into test machine
    • Comment out Syncing to Pardot section so changes happen only in local db.
    • Start a screen section so job will still run even connection to test server breaks.
    • Run bin/cron/build_contact_rollups.
    • (Detach = ctrl+a+d, resume section = screen -r <sessions_name>, list = screen -ls)

Note

I add all PLC to this PR since Contact Rollups is in PLC land now and anyone of us may have to work on it in the future.

TO DO

  • Remove stdout prints before merging.

@hacodeorg hacodeorg requested a review from agealy June 12, 2019 01:08
@hacodeorg hacodeorg marked this pull request as ready for review June 12, 2019 01:33
@hacodeorg hacodeorg changed the title Ha/pardot log Measure contact rollups job Jun 12, 2019
lib/cdo/log_object.rb Outdated Show resolved Hide resolved
lib/cdo/log_object.rb Outdated Show resolved Hide resolved
bin/cron/build_contact_rollups Outdated Show resolved Hide resolved
bin/cron/build_contact_rollups Show resolved Hide resolved
Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Hooray for instrumentation!

require 'cdo/log_collector'

class LogCollectorTest < Minitest::Test
def test_time_a_function
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use TimeCop to test that LogCollector correctly measures the amount of time it took the function to execute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will keep this idea for future improvement.
I'm looking at examples of Timecop in our code base; 2 common functions areTimecop.freeze(time) and Timecop.travel and it is not obvious to me how to measure execution time?

Copy link
Contributor

@sureshc sureshc Jun 17, 2019

Choose a reason for hiding this comment

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

Makes sense. I think we could freeze time before invoking a method that is wrapped in the log collector, and inside the wrapped method we could travel forward. That might enable us to assert that the log collector correctly measured how much time elapsed during execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

@hacodeorg hacodeorg merged commit d70b06a into staging Jun 17, 2019
@hacodeorg hacodeorg deleted the ha/pardot-log branch June 21, 2019 22:07
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.

None yet

6 participants