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

feat(study-rooms): serve slots data #75

Merged
merged 8 commits into from
Jan 15, 2025
Merged

feat(study-rooms): serve slots data #75

merged 8 commits into from
Jan 15, 2025

Conversation

laggycomputer
Copy link
Member

@laggycomputer laggycomputer commented Jan 12, 2025

Description

Adds study room slots and availability to study room endpoint.

As previously implemented in the data pipeline, availability is available in on the days between the day of the current time in UCI's timezone (America/Los_Angeles) and exactly 3 days later, inclusive. Data is updated every 2 minutes.

This PR simply exposes that data. We use ISO 8601 dates (most precisely, z.string().datetime({ local: true })) which are given with the Z indicating UTC but should be interpreted as being in UCI's timezone. We ask Postgres to interpret the TIMESTAMP NO TIMEZONE in America/Los_Angeles and format as zoned ISO 8601, avoiding adding a JS timezone library.

Note: The slots field on returned study rooms is now guaranteed to exist. This is not a breaking change.

We use a materialized view to precompute the JSON structure of the response. The refreshing of this view is relatively light and is done as part of the study room and slot scraping process every 2 minutes. However, it is not completely trivial; see below demonstrating that it produces a significant speed improvement per request which will accrue over time. This design decision is a database schema change. Since we have this JSON structure beforehand, we need only SELECT with conditions when serving a response.

Related Issue

Feature update as requested in #46. Also closes the inconsistency raised in #74 since study room data was one of the offenders and is within the jurisdiction of this PR. It doesn't.

Motivation and Context

be me
want study room slots
no study room slots

mfw

How Has This Been Tested?

Tested with Postman on a local deployment.

Screenshots (if appropriate):

Old screenshots (before timezone correction):

REST is functional:
Screenshot_20250111_192159

GraphQL is functional:
Screenshot_20250111_192242

New screenshots (latest):

Screenshot_20250114_145739

Screenshot_20250114_150118

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code involves a change to the database schema.
  • My code requires a change to the documentation.

@laggycomputer laggycomputer temporarily deployed to staging-75 January 12, 2025 03:23 — with GitHub Actions Inactive
@andrew-wang0 andrew-wang0 self-requested a review January 13, 2025 21:47
Copy link
Member

@andrew-wang0 andrew-wang0 left a comment

Choose a reason for hiding this comment

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

As discussed, the study room slots start and end times being displayed like UTC time while being in PST/PDT could be confusing.

We've resolved to use true UTC time instead 👌

@laggycomputer
Copy link
Member Author

To be precise, I'm going to specify the correct UTC offset in the response.

@laggycomputer laggycomputer temporarily deployed to staging-75 January 14, 2025 01:02 — with GitHub Actions Inactive
@laggycomputer laggycomputer temporarily deployed to staging-75 January 14, 2025 05:11 — with GitHub Actions Inactive
@laggycomputer laggycomputer temporarily deployed to staging-75 January 14, 2025 05:12 — with GitHub Actions Inactive
@laggycomputer
Copy link
Member Author

laggycomputer commented Jan 14, 2025

Without fully redoing tests, the correct offset is now specified:
Screenshot_20250113_211320

We add moment-timezone with this PR; this is a change in the convention of this project to use vanilla Date and may require some further work to re-standardize.

EDIT: This PR now uses Postgres to handle timezones.

@laggycomputer laggycomputer marked this pull request as draft January 14, 2025 05:15
@laggycomputer
Copy link
Member Author

Demoting to draft; I've just remembered Postgres supports JSON aggregation and benchmarking should be done to determine whether it is a better (faster) approach.

@laggycomputer laggycomputer temporarily deployed to staging-75 January 14, 2025 06:03 — with GitHub Actions Inactive
@laggycomputer laggycomputer temporarily deployed to staging-75 January 14, 2025 06:07 — with GitHub Actions Inactive
@laggycomputer laggycomputer temporarily deployed to staging-75 January 14, 2025 19:03 — with GitHub Actions Inactive
@laggycomputer
Copy link
Member Author

laggycomputer commented Jan 14, 2025

Some benchmarking:

  • Aggregation of slots in JS takes 72.8±7.5ms per request.
  • Aggregation of slots in SQL on every request takes 76.8±7.6ms per request.
  • Aggregation of slots in a SQL materialized view updated on every scrape takes 61.4±6.4ms per request (p < 1e-4 improvement over JS).

Given the following conditions:

  • Mostly quiet local system.
  • Middleware responsible for validating keys was disabled to reduce variance. Ratelimiting middleware was disabled to avoid spoiling the tests with 429s, since they by nature need to execute 1000 consecutive requests successfully.
  • Testing method was hyperfine --warmup 10 -m 1000 "curl localhost:8787/v2/rest/studyRooms", i.e. n=1000 with 10 warmup runs.

We currently used a materialized view for this PR. Promoting out of draft status now that this analysis is complete.

@laggycomputer laggycomputer marked this pull request as ready for review January 14, 2025 22:20
@ecxyzzy ecxyzzy linked an issue Jan 14, 2025 that may be closed by this pull request
@andrew-wang0
Copy link
Member

andrew-wang0 commented Jan 14, 2025

Wow mindblowing work on this, will dig in to test 😎 👌

Copy link
Member

@andrew-wang0 andrew-wang0 left a comment

Choose a reason for hiding this comment

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

Looks good! A few format errors but other than that it's good to go

apps/api/src/services/study-rooms.ts Show resolved Hide resolved
@laggycomputer laggycomputer temporarily deployed to staging-75 January 15, 2025 03:52 — with GitHub Actions Inactive
@andrew-wang0 andrew-wang0 temporarily deployed to staging-75 January 15, 2025 03:53 — with GitHub Actions Inactive
Copy link
Member

@andrew-wang0 andrew-wang0 left a comment

Choose a reason for hiding this comment

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

Another quick nitpick

@laggycomputer laggycomputer temporarily deployed to staging-75 January 15, 2025 04:03 — with GitHub Actions Inactive
Copy link
Member

@andrew-wang0 andrew-wang0 left a comment

Choose a reason for hiding this comment

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

Looks good!

@andrew-wang0 andrew-wang0 merged commit e1eccc4 into main Jan 15, 2025
1 check passed
@andrew-wang0 andrew-wang0 deleted the study-room-slots branch January 15, 2025 04:08
andrew-wang0 pushed a commit that referenced this pull request Jan 15, 2025
## Description

Followup to #75, fixing study room slot times being rendered in
Postgres's local timezone.
We simply set the timezone on the connection acquired by the scraper to
`America/Los_Angeles`.

## Related Issue

Resolves #84. We both specify the intended behavior from #75 in the
example and actually fulfill it.

## Motivation and Context

## How Has This Been Tested?

Tested by setting Postgres's timezone to `America/New_York`, running the
scraper, then testing the API request.

## Screenshots (if appropriate):

Despite forcing Postgres's global timezone to change, the timezone is
correct since we set it on the session which takes precedence over the
global setting:


![Screenshot_20250115_110206](https://github.com/user-attachments/assets/97c1badd-5c8a-44af-9b1c-8daddf03421e)

We know this is effective because setting a different timezone for the
scraper's SQL session uses that timezone instead:


![Screenshot_20250115_110404](https://github.com/user-attachments/assets/371a8cda-3751-461b-b2b0-69092e441b55)

So if we force `America/Los_Angeles` on the scraper session, it will
give the correct offset regardless of Postgres or system timezone.

## Types of changes

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)

## Checklist:

- [ ] My code involves a change to the database schema.
- [ ] My code requires a change to the documentation.
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.

Return availability in studyRoom endpoint
2 participants