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

alert if we run low on codes #19532

Merged
merged 2 commits into from Dec 8, 2017
Merged

alert if we run low on codes #19532

merged 2 commits into from Dec 8, 2017

Conversation

Bjvanminnen
Copy link
Contributor

If we start to run low on discount codes, make sure HoneyBadger lets us know about it.

Not too sure of the best way of testing this. I made sure that things still work on my local env, but of course can't validate the HB error itself. Presumably I could test this on staging (and maybe staging-next) by using my one-off script to create < 20 fake codes.

first
limit(20)

if codes.length < 20
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure length is the right call here? It looks like size might be a better choice.

limit(20)

if codes.length < 20
Honeybadger.notify("Fewer than 20 remaining circuit playground discount codes",
Copy link
Contributor

Choose a reason for hiding this comment

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

This call looks right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, without an error (exception) object, the call is slightly different in order to get it to show up the way we expect. This will work, but I don't think the message will show up in the expected place. @drewsamnick and I went through this recently and came up with this

Honeybadger.notify(
error_message: "Overwriting passed in data for new SchoolInfo",
error_class: "SchoolInfo.sync_from_schools",
context: {
original_input: original,
school_id: school.id
}

The main difference is you want to add an error_class, which here is just an arbitrary string, and explicitly specify that the supplied message here is an error_message.

If you want, you can test this by setting up a personal honeybadger project and logging there from development. I did that a couple years ago when I set up the cronjob-honeybadger logging (aka cronbadger).

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize I sent you examples that supplied a rescued error, so those were slightly misleading in this context.

@Bjvanminnen
Copy link
Contributor Author

This has now been tested. For future reference my steps were:

  1. Create a new HB project
  2. In locals.yml set dashboard_honeybadger_api_key to the API key for the new project (not to be confused with the HB token)
  3. In dashboard/config/honeybadger.yml set report_data: true, otherwise HB automatically ignores development environment.

@@ -20,11 +20,23 @@ def self.claim(full_discount)

code = nil
Retryable.retryable(on: ActiveRecord::RecordNotSaved) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this cause an infinite loop if the update_all fails? In particular, does this cause us to repeatedly notify HB? Perhaps a number of retries should be specified?

Copy link
Contributor

Choose a reason for hiding this comment

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

Retryable.retryable defaults to 2 tries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, useful to know. LGTM.

@Bjvanminnen Bjvanminnen merged commit dc36485 into staging-next Dec 8, 2017
@Bjvanminnen Bjvanminnen deleted the honeyBadgerAlert branch December 8, 2017 17:12
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

4 participants