Skip to content

LG-4305: Log new event in case of lockout from proofing#4803

Merged
aduth merged 4 commits intomainfrom
aduth-lg-4305-log-throttle
Mar 23, 2021
Merged

LG-4305: Log new event in case of lockout from proofing#4803
aduth merged 4 commits intomainfrom
aduth-lg-4305-log-throttle

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Mar 15, 2021

Why: As a login.gov developer, I want to see an event in the event log that indicates that a user was locked out for 6 hours from proofing along with the relevant data points that directly resulted in that lockout, so that I can troubleshoot any issues reported by end users and pinpoint exactly what caused the lock out.

Copy link
Contributor

Choose a reason for hiding this comment

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

all the other events have lowercase rate limited, can we do that here too? Since cloudwatch regexes are case-sensitive by default it would make it easier for us to find all the rate limits with one query if we wanted to

Suggested change
IAL2_RECOVERY_REQUEST_RATE_LIMIT_TRIGGERED = 'IAL2 Recovery Request Rate Limited'.freeze
IAL2_RECOVERY_REQUEST_RATE_LIMIT_TRIGGERED = 'IAL2 Recovery Request rate limited'.freeze

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was an unfortunate choice between consistency amongst the peer of "IAL2 Recovery Request" events vs. "rate limited" events. You raise a good point about being able to query rate limited events.

I was actually beginning to explore an alternative approach where the logging happens as part of the throttling behaviors (e.g. in or as a result of Throttler::IsThrottledElseIncrement and Throttler::Increment). To your point, this would have the added benefit of there being only one event associated with rate limits being triggered.

@aduth aduth force-pushed the aduth-lg-4305-log-throttle branch from 0377296 to b09153f Compare March 18, 2021 13:52
@aduth
Copy link
Contributor Author

aduth commented Mar 18, 2021

Exploring a couple alternatives:

  • afdc8d3 at least consolidates to a single event, per point above, to optimize for discoverability of throttle trigger events
  • b09153f moves logging behavior into the Throttler service module
    • This feels better in the sense that the throttler is responsible for its own logging, but doesn't feel so great to be passing around analytics like this. I wonder if there's an argument to be made to allow the service to create its own Analytics instance, regardless if we can carry through all the "base" analytics values like those derived from sp or request.

cc also @amathews-fs , re: our chat yesterday.

Still tinkering here, and finalizing details around tests.

@aduth aduth marked this pull request as ready for review March 22, 2021 18:00
aduth added 4 commits March 23, 2021 08:28
**Why**: As a login.gov developer, i want to see an event in the event log that indicates that a user was locked out for 6 hours from proofing along with the relevant data points that directly resulted in that lockout, so that I can troubleshoot any issues reported by end users and pinpoint exactly what caused the lock out.
**Why**: Easier queryability of events, fewer and more consistent events, compatibility with potential future refactor to move event logging into throttle service or model.
**Why**: Encapsulate behaviors of throttling within existing service behaviors
@aduth aduth force-pushed the aduth-lg-4305-log-throttle branch from 28dd326 to dd52cee Compare March 23, 2021 12:50
@aduth aduth merged commit 7c29df9 into main Mar 23, 2021
@aduth aduth deleted the aduth-lg-4305-log-throttle branch March 23, 2021 13:05
@aduth
Copy link
Contributor Author

aduth commented Mar 23, 2021

I created a follow-up ticket LG-4404 to log events for non-proofing throttle types.

aduth added a commit that referenced this pull request Mar 24, 2021
aduth added a commit that referenced this pull request Mar 25, 2021
* Revert "LG-4305: Log new event in case of lockout from proofing (#4803)"

This reverts commit 7c29df9.

* Revert throttle logging from #4820

* Revert throttler logging from #4827
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