Skip to content

log when resolution and address proofing are throttled (LG-4430)#4858

Merged
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/throttle-updates
Mar 31, 2021
Merged

log when resolution and address proofing are throttled (LG-4430)#4858
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/throttle-updates

Conversation

@mitchellhenke
Copy link
Contributor

Adds a couple more throttle events, and some additional detail to be able to distinguish events using the same throttle key

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM


def confirm_step_allowed
redirect_to_fail_url if step_attempts_exceeded?
if step_attempts_exceeded?
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it's in the AC but how about logic to only log it once when the limit is reached? (vs on every retry)

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought we can just query distinct. Probably better to log it.

Copy link
Contributor

@stevegsa stevegsa Mar 31, 2021

Choose a reason for hiding this comment

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

Flip flopping again. Def better to know if they hit the limit separately (and only once for the window). Hitting the limit, waiting the window and getting throttled again should be separate from 2 tries when throttled in one window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have a good way to only log once unfortunately. We initially had the event fire when the throttler incremented to be on the final attempt, but since it's possible for the final attempt to succeed that wasn't a great way to capture that someone was throttled. We typically only have throttler checks at the beginning of an action, which means it can happen multiple times :/

For reference, this builds on work from #4829

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have two separate events or one event and a boolean which indicates they are retrying and still throttled

Copy link
Contributor

@stevegsa stevegsa Mar 31, 2021

Choose a reason for hiding this comment

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

"We don't have a good way to only log once unfortunately." - We'd have to keep incrementing the throttle and do a check but that's a big change. I'll go ahead and approve

Comment on lines +15 to +18
analytics.track_event(
Analytics::THROTTLER_RATE_LIMIT_TRIGGERED,
throttle_type: :idv_resolution,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional to omit the step_name from this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check happens before a step or anything is reached, so step_name doesn't exist

@mitchellhenke mitchellhenke merged commit bbad7a9 into main Mar 31, 2021
@mitchellhenke mitchellhenke deleted the mitchellhenke/throttle-updates branch March 31, 2021 18:00
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.

4 participants