Trending Courses/Professors Feature#1231
Conversation
ArislanT
added
Feature
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces analytics tracking functionality to the application. Environment variables for DynamoDB-based analytics are added to the configuration template, and a new module implements asynchronous view recording for courses and instructors using a thread pool executor with event queueing and backpressure management. Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant Queue as ThreadPoolExecutor
participant Executor as Worker Thread
participant DDB as DynamoDB
App->>Queue: record_course_view(course_id)<br/>or record_instructor_view(instructor_id)
alt Pending < MAX_BACKLOG
Queue->>Queue: Queue task (_send_to_dynamo)
Queue->>Executor: Execute task
Executor->>DDB: Update item (PK: entity_type:id,<br/>SK: YYYY-MM-DD)
Executor->>DDB: Increment view_count, set expires_at
alt Success
DDB-->>Executor: Success
else Exception
Executor-->>Executor: Log warning, continue
end
else Pending >= MAX_BACKLOG
Queue-->>App: Drop event (backpressure)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tcf_website/analytics_utils.py`:
- Around line 51-62: The analytics init should detect an explicit "disabled"
fast path instead of treating missing env vars as an error: check the relevant
env flags/keys before creating resources and if analytics are disabled set a
clear disabled state (e.g., _SESSION=None, _DYNAMODB=None, and a new
_ANALYTICS_DISABLED=True) and return early so you never touch or start _EXECUTOR
or enqueue background tasks; update get_analytics_health() to report "disabled"
when _ANALYTICS_DISABLED is True and guard the code paths around _EXECUTOR, the
background enqueue logic (the block around lines 109-145) and the subsequent
task submission (lines 165-189) to no-op when disabled instead of logging
failures.
- Around line 14-17: The project is missing the boto3 runtime dependency
referenced by the import boto3 in tcf_website/analytics_utils.py (which causes
tcf_website/views/browse.py to fail at import/time); add "boto3" to the
top-level dependencies list in pyproject.toml (same section where other packages
are listed) so the package is installed for runtime and imports succeed.
In `@tcf_website/views/browse.py`:
- Around line 294-295: The analytics calls (record_course_view(course.id) and
the course_instructor call site) are currently executed on every request method
in instructor_view and other handlers, skewing metrics; update those sites to
only run for successful GET page renders by either wrapping them with a guard
that checks request.method == "GET" before calling record_course_view(course.id)
and course_instructor, or move the calls to after the successful render(...)
return so they only execute on successful GET responses; reference the
instructor_view function and the call sites where record_course_view(course.id)
and course_instructor are invoked and apply the GET-only guard or relocation
consistently at all three locations mentioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3121c38a-84d5-4fa7-9a2a-510d54d0748b
📒 Files selected for processing (3)
.env.exampletcf_website/analytics_utils.pytcf_website/views/browse.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tcf_website/analytics_utils.py (1)
87-89: Avoid repeated env/table resolution in the hot path.Resolving
DYNAMODB_TABLE_NAMEandTable(...)on every event adds avoidable overhead under load; cache once at init and reuse.Refactor sketch
+# Configuration +_TABLE_NAME = env("DYNAMODB_TABLE_NAME", default="trending_analytics") +_TABLE = None @@ if access_key and secret_key: _DYNAMODB = boto3.Session( @@ ).resource("dynamodb", config=_BOTO_CONFIG) + _TABLE = _DYNAMODB.Table(_TABLE_NAME) _ANALYTICS_ENABLED = True @@ def _send_to_dynamo(entity_type: str, entity_id: int) -> None: @@ - if _DYNAMODB is None: + if _TABLE is None: return @@ - table_name = env("DYNAMODB_TABLE_NAME", default="trending_analytics") - table = _DYNAMODB.Table(table_name) - @@ - table.update_item( + _TABLE.update_item(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/analytics_utils.py` around lines 87 - 89, The code repeatedly calls env("DYNAMODB_TABLE_NAME") and _DYNAMODB.Table(table_name) on each event, which is expensive; move the resolution and Table creation to module/init time and reuse a cached value. Create a module-level constant (e.g., DYNAMODB_TABLE_NAME or CACHED_TABLE_NAME) and a single cached table object (e.g., CACHED_DDB_TABLE = _DYNAMODB.Table(DYNAMODB_TABLE_NAME)) and update code that currently computes table_name/table to use CACHED_DDB_TABLE instead. Ensure any tests or code that overwrite env in test runs can still patch the cached objects as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tcf_website/analytics_utils.py`:
- Line 31: The TTL configuration _TTL_DAYS (env key ANALYTICS_TTL_DAYS) can be
non-positive which makes computed expires_at immediate/past; validate and clamp
it at load time: when reading _TTL_DAYS, if value is None or <= 0, set it to a
safe minimum (e.g. 1) and optionally log a warning; ensure any use sites that
compute expires_at rely on this validated _TTL_DAYS so fresh analytics rows are
not purged immediately (refer to the module-level _TTL_DAYS constant and the
code that computes expires_at).
- Around line 181-182: The health docstring in analytics_utils.py is out of
sync: it mentions status values "healthy/session_failed" but the code actually
returns "healthy/disabled"; update the docstring(s) that describe the health
contract to list the correct status values ("healthy" and "disabled") and any
other returned fields (e.g., "pending_tasks") so they match the implementation;
ensure both occurrences (the docstring around line ~181 and the duplicate at
~200) are changed to reflect the actual return values and wording used by the
health-related function.
---
Nitpick comments:
In `@tcf_website/analytics_utils.py`:
- Around line 87-89: The code repeatedly calls env("DYNAMODB_TABLE_NAME") and
_DYNAMODB.Table(table_name) on each event, which is expensive; move the
resolution and Table creation to module/init time and reuse a cached value.
Create a module-level constant (e.g., DYNAMODB_TABLE_NAME or CACHED_TABLE_NAME)
and a single cached table object (e.g., CACHED_DDB_TABLE =
_DYNAMODB.Table(DYNAMODB_TABLE_NAME)) and update code that currently computes
table_name/table to use CACHED_DDB_TABLE instead. Ensure any tests or code that
overwrite env in test runs can still patch the cached objects as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66f38d40-f407-4cdc-a2f3-ff25761fd8d3
📒 Files selected for processing (1)
tcf_website/analytics_utils.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tcf_website/analytics_utils.py`:
- Around line 57-61: The module-level _DYNAMODB resource is not thread-safe and
is being shared by worker threads via _EXECUTOR and used in _send_to_dynamo (and
where _DYNAMODB.Table() is called); replace it with a thread-safe pattern by
either creating a module-level boto3.client("dynamodb", ...) (clients are
thread-safe) and update _send_to_dynamo and any table operations to use the
client API, or instantiate a new boto3.resource(...) inside the worker (inside
_send_to_dynamo) and call resource.Table(...) there so each thread uses its own
resource; update references to _DYNAMODB and _DYNAMODB.Table() accordingly
(e.g., in _send_to_dynamo) to use the chosen approach.
In `@tcf_website/views/index.py`:
- Around line 120-124: The current logic sorts raw rows in items by individual
row view_count then picks top 5, which lets one entity with multiple daily rows
occupy multiple slots; instead aggregate view_count by entity key (pk) first,
summing int(x.get("view_count",0)) for each unique pk, then produce a list of
aggregated items and sort that aggregated list by the summed view_count
descending to pick the top 5 (then derive course_ids from the aggregated pk
values); apply the same aggregation change to the analogous helper
get_trending_instructors() so it also sums view_count per pk before ranking.
- Around line 27-32: The module currently creates a module-global boto3 resource
and Table objects (_DYNAMODB and _TABLE) which are then used by
scan_table_paginated(), causing thread-safety issues; fix this by instantiating
a fresh DynamoDB resource or a low-level boto3.client inside the request handler
(the view function that calls scan_table_paginated()) and passing the resulting
Table or client into scan_table_paginated() instead of referencing the
module-global _TABLE, or refactor scan_table_paginated() to accept a
table/client parameter and create the resource there per-request; update all
references to _TABLE and _DYNAMODB to use the per-request object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 546a00a8-1097-4fbc-a15d-170de4b02bc9
📒 Files selected for processing (2)
tcf_website/analytics_utils.pytcf_website/views/index.py
| _DYNAMODB = boto3.Session( | ||
| aws_access_key_id=access_key, | ||
| aws_secret_access_key=secret_key, | ||
| region_name=env("AWS_REGION", default="us-east-1"), | ||
| ).resource("dynamodb") | ||
| _TABLE = _DYNAMODB.Table(table_name) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Are boto3 DynamoDB resource/Table objects safe to share across concurrent Django request threads? Please cite the official boto3 docs and note the recommended thread-safe pattern.
💡 Result:
No, boto3 DynamoDB resource and Table objects are not safe to share across concurrent Django request threads. Official boto3 documentation explicitly states that resource instances are not thread safe and should not be shared across threads or processes due to containing additional metadata that cannot be shared. This applies to DynamoDB resources like dynamodb = boto3.resource('dynamodb') and table = dynamodb.Table('my-table'). The recommended thread-safe pattern is to create a new resource (or client) for each thread by first creating a new session per thread: session = boto3.session.Session dynamodb = session.resource('dynamodb') table = dynamodb.Table('my-table') In a Django context with concurrent request threads (e.g., via Gunicorn uWSGI with threads), this means creating the resource or Table object anew in each view or request handler, rather than reusing a global or singleton instance. Note that while low-level clients (boto3.client('dynamodb')) are generally thread-safe once created from a session, the question specifically concerns resource/Table objects, which are not. For efficiency in Django, consider using a middleware or context manager to create per-request sessions/resources, but avoid global sharing.
Citations:
- 1: https://boto3.amazonaws.com/v1/documentation/api/1.14.31/guide/resources.html?highlight=thread%20safe
- 2: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/programming-with-python.html
- 3: https://boto3.amazonaws.com/v1/documentation/api/1.36.26/guide/session.html
Avoid module-global DynamoDB Table objects in request code.
_TABLE is initialized once at import time and shared across all concurrent requests via scan_table_paginated(). In threaded Django deployments, DynamoDB resource/Table objects are not thread-safe and will cause issues. Create a new resource inside the request handler or use the low-level boto3 client instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tcf_website/views/index.py` around lines 27 - 32, The module currently
creates a module-global boto3 resource and Table objects (_DYNAMODB and _TABLE)
which are then used by scan_table_paginated(), causing thread-safety issues; fix
this by instantiating a fresh DynamoDB resource or a low-level boto3.client
inside the request handler (the view function that calls scan_table_paginated())
and passing the resulting Table or client into scan_table_paginated() instead of
referencing the module-global _TABLE, or refactor scan_table_paginated() to
accept a table/client parameter and create the resource there per-request;
update all references to _TABLE and _DYNAMODB to use the per-request object.
| # Safe sort that handles missing view counts | ||
| top_items = sorted(items, key=lambda x: int(x.get("view_count", 0)), reverse=True)[ | ||
| :5 | ||
| ] | ||
| course_ids = [int(item["pk"].split(":")[1]) for item in top_items] |
There was a problem hiding this comment.
Aggregate per entity before picking the top 5.
The writer stores one row per entity per day (pk + sk=date; see tcf_website/analytics_utils.py Line 98), but these helpers rank raw rows. That makes "trending" use the best single day, not the total within the TTL window, and the same entity can consume multiple slots. Sum view_count by pk first, then rank the aggregated totals.
Possible fix
+from collections import defaultdict
...
- top_items = sorted(items, key=lambda x: int(x.get("view_count", 0)), reverse=True)[
- :5
- ]
- course_ids = [int(item["pk"].split(":")[1]) for item in top_items]
+ totals = defaultdict(int)
+ for item in items:
+ try:
+ totals[item["pk"]] += int(item.get("view_count", 0))
+ except (KeyError, TypeError, ValueError):
+ continue
+ course_ids = [
+ int(pk.split(":", 1)[1])
+ for pk, _ in sorted(totals.items(), key=lambda kv: kv[1], reverse=True)[:5]
+ ]Apply the same pattern in get_trending_instructors().
Also applies to: 153-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tcf_website/views/index.py` around lines 120 - 124, The current logic sorts
raw rows in items by individual row view_count then picks top 5, which lets one
entity with multiple daily rows occupy multiple slots; instead aggregate
view_count by entity key (pk) first, summing int(x.get("view_count",0)) for each
unique pk, then produce a list of aggregated items and sort that aggregated list
by the summed view_count descending to pick the top 5 (then derive course_ids
from the aggregated pk values); apply the same aggregation change to the
analogous helper get_trending_instructors() so it also sums view_count per pk
before ranking.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tcf_website/analytics_utils.py (2)
18-18:⚠️ Potential issue | 🟡 MinorClamp
ANALYTICS_TTL_DAYSto a positive value.Line 18 accepts non-positive TTL values, which makes Line 53 compute immediate/past expiration and risks premature item eviction.
Suggested hardening
-_TTL_DAYS = env.int("ANALYTICS_TTL_DAYS", default=7) +_ttl_days_cfg = env.int("ANALYTICS_TTL_DAYS", default=7) +if _ttl_days_cfg <= 0: + logger.warning("Invalid ANALYTICS_TTL_DAYS=%s; defaulting to 1", _ttl_days_cfg) +_TTL_DAYS = _ttl_days_cfg if _ttl_days_cfg > 0 else 1Also applies to: 53-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/analytics_utils.py` at line 18, The _TTL_DAYS configuration can be zero or negative which causes immediate/past expirations; update the initialization of _TTL_DAYS to clamp to a positive minimum (e.g., use max(1, env.int("ANALYTICS_TTL_DAYS", default=7)) or validate and raise) so any downstream expiration computation (where _TTL_DAYS is used to compute expiry) always receives a positive TTL.
67-77:⚠️ Potential issue | 🟠 MajorHandle executor submission failures to avoid leaked
_pendingand request-path exceptions.At Line 68 you increment
_pendingbefore submit, but Line 77 can raise (e.g., executor shutdown/broken pool). That leaves_pendingstuck and can propagate an exception to page-render paths.Suggested fix
def _fire_and_forget(entity_type: str, entity_id: int) -> None: if not _ANALYTICS_ENABLED: return global _pending with _pending_lock: if _pending >= _MAX_BACKLOG: return _pending += 1 @@ - _EXECUTOR.submit(task_wrapper) + try: + _EXECUTOR.submit(task_wrapper) + except RuntimeError: + with _pending_lock: + _pending -= 1 + logger.debug("Analytics executor unavailable; dropped %s:%s", entity_type, entity_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/analytics_utils.py` around lines 67 - 77, You increment the global _pending before calling _EXECUTOR.submit(task_wrapper) which can raise (executor shutdown), leaking _pending and surfacing exceptions to request paths; fix by surrounding the submit call with try/except so that if _EXECUTOR.submit(task_wrapper) raises you immediately decrement _pending under _pending_lock (and then re-raise or handle/log the exception), or alternatively only increment _pending after a successful submit; reference symbols: _pending, _pending_lock, _EXECUTOR, task_wrapper, and _send_to_dynamo.
🧹 Nitpick comments (1)
.env.example (1)
52-58: DocumentANALYTICS_TTL_DAYSin the analytics env block.
analytics_utils.pyreadsANALYTICS_TTL_DAYS; including it here improves operability and avoids hidden defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 52 - 58, The .env.example analytics block is missing the ANALYTICS_TTL_DAYS variable used by analytics_utils.py; add a commented line for ANALYTICS_TTL_DAYS under the Trending Analytics section with a short description and a recommended default (e.g., number of days to retain analytics, e.g., 30) so operators see the setting and there are no hidden defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 58: Add a single trailing newline at the end of .env.example so the file
ends with a blank line (this will satisfy dotenv-linter's EndingBlankLine rule);
simply ensure the file ends after the commented DYNAMODB_TABLE_NAME line with
one final newline character.
---
Duplicate comments:
In `@tcf_website/analytics_utils.py`:
- Line 18: The _TTL_DAYS configuration can be zero or negative which causes
immediate/past expirations; update the initialization of _TTL_DAYS to clamp to a
positive minimum (e.g., use max(1, env.int("ANALYTICS_TTL_DAYS", default=7)) or
validate and raise) so any downstream expiration computation (where _TTL_DAYS is
used to compute expiry) always receives a positive TTL.
- Around line 67-77: You increment the global _pending before calling
_EXECUTOR.submit(task_wrapper) which can raise (executor shutdown), leaking
_pending and surfacing exceptions to request paths; fix by surrounding the
submit call with try/except so that if _EXECUTOR.submit(task_wrapper) raises you
immediately decrement _pending under _pending_lock (and then re-raise or
handle/log the exception), or alternatively only increment _pending after a
successful submit; reference symbols: _pending, _pending_lock, _EXECUTOR,
task_wrapper, and _send_to_dynamo.
---
Nitpick comments:
In @.env.example:
- Around line 52-58: The .env.example analytics block is missing the
ANALYTICS_TTL_DAYS variable used by analytics_utils.py; add a commented line for
ANALYTICS_TTL_DAYS under the Trending Analytics section with a short description
and a recommended default (e.g., number of days to retain analytics, e.g., 30)
so operators see the setting and there are no hidden defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3521b372-49f0-41ee-86dd-2ef94e6492a6
📒 Files selected for processing (2)
.env.exampletcf_website/analytics_utils.py
| # AWS_ANALYTICS_ACCESS_KEY_ID= | ||
| # AWS_ANALYTICS_SECRET_ACCESS_KEY= | ||
| # AWS_REGION=us-east-1 | ||
| # DYNAMODB_TABLE_NAME=trending_analytics No newline at end of file |
There was a problem hiding this comment.
Add a trailing blank line at EOF to satisfy dotenv-linter.
Static analysis flags EndingBlankLine on Line 58.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 58-58: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example at line 58, Add a single trailing newline at the end of
.env.example so the file ends with a blank line (this will satisfy
dotenv-linter's EndingBlankLine rule); simply ensure the file ends after the
commented DYNAMODB_TABLE_NAME line with one final newline character.
|
Had to make changes to refactor towards the new UI changes. Should be complete towards it |
|
Superseded by #1248 which consolidates backend and frontend into one PR |
1 participant
GitHub Issues addressed
What I did
Changes:
Infrastructure:
Screenshots
Testing
I utilized Locust to verify the stability of the asynchronous pipeline under high-concurrency conditions within the local Docker environment.
The system maintained total stability and was reliable. Latency remained consistent until local hardware limits were reached, confirming the Django application logic is not an issue.
The small failure rate, which was 7 failures out of 9,900, confirms that the ThreadPoolExecutor backlog limits were reached. This validates the system's ability to prioritize site stability over analytic precision during extreme traffic spikes by dropping excess events rather than blocking the main request cycle.
(The seven views were successfully rendered for users but just not counted in analytics)
Refinements Following Automated Review & Testing
Primary Refinement:
Other Changes:
Questions/Discussions/Notes
Summary by CodeRabbit
Release Notes