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

Mirror Session Deletion to Redis Store #57972

Merged
merged 2 commits into from Apr 12, 2024

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Apr 11, 2024

Follow-up to #57562, which started writing session data to Redis with the goal of giving us a chance to analyze real-world usage in advance of a switch to actually start using Redis as the source of truth.

For the most part, that's working, but there are currently a couple of issues with that implementation which both affect the total amount of data being stored, and render that information inaccurate.

First, we aren't deleting old sessions from Redis, so aren't capturing the extent to which users logging out will reduce total data storage. To resolve that, this PR simply extends the delete_session method to begin mirroring deletions in Redis the same as we mirror writes.

Second, we're actually writing more data than we need to; I mistakenly thought we were going to need to manually serialize and deserialize the Ruby Hashes that our session data is stored in, and chose to do so with Marshal, which is much less space-efficient than simply passing the Hash directly to Redis to store. In my local testing, a Hash for a freshly-logged-in session is 168 bytes in memory, but the Marshal serialization takes 424 bytes.

Simplifying the write logic and enabling deletion logic should improve the accuracy of our metrics.

Testing story

Tested locally that logging in, viewing some pages which store information in the session, and then logging back out all work as expected.

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Follow-up to #57562,
which started writing session data to Redis with the goal of giving us a
chance to analyze real-world usage in advance of a switch to actually
start using Redis as the source of truth.

For the most part, that's working, but there are currently a couple of
issues with that implementation which both affect the total amount of
data being stored, and render that information inaccurate.

First, we aren't deleting old sessions from Redis, so aren't capturing
the extent to which users logging out will reduce total data storage. To
resolve that, this PR simply extends the
[`delete_session`](https://github.com/rails/rails/blob/v6.1.7.7/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb#L63-L68)
method to begin mirroring deletions in Redis the same as we mirror
writes.

Second, we're actually writing more data than we need to; I mistakenly
thought we were going to need to manually serialize and deserialize the
Ruby Hashes that our session data is stored in, and chose to do so with
`Marshal`, which is much less space-efficient than simply passing the
Hash directly to Redis to store. In my local testing, a Hash for a
freshly-logged-in session is 168 bytes in memory, but the `Marshal`
serialization takes 424 bytes.

Simplifying the write logic and enabling deletion logic should improve
the accuracy of our metrics.
@Hamms Hamms marked this pull request as ready for review April 11, 2024 23:18
@Hamms Hamms merged commit f0bc622 into staging Apr 12, 2024
2 checks passed
@Hamms Hamms deleted the mirror-session-deletion-to-redis-store branch April 12, 2024 17:35
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

3 participants