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

Fix ctx.features missing in fakeload #225

Merged
merged 3 commits into from
Mar 18, 2025
Merged

Conversation

cdrini
Copy link
Collaborator

@cdrini cdrini commented Mar 17, 2025

Closes internetarchive/openlibrary#10341

Tested locally with some debugging print statements:

diff --git a/conf/openlibrary.yml b/conf/openlibrary.yml
index b46c4c427..e05d61ba7 100644
--- a/conf/openlibrary.yml
+++ b/conf/openlibrary.yml
@@ -13,7 +13,7 @@ support_case_control_address: "support@archive.org"
 smtp_server: localhost
 
 dummy_sendmail: True
-debug: True
+# debug: True
 
 coverstore_url: http://covers:7075
 # URL to use inside HTML files; must be publicly accessible from
@@ -56,7 +56,10 @@ http_request_timeout: 10
 
 features:
     cache_most_recent: enabled
-    debug: enabled
+    debug:
+        filter: queryparam
+        name: debug
+        value: "true"
     dev: enabled
     history_v2: admin
     inlibrary: enabled
diff --git a/openlibrary/plugins/openlibrary/home.py b/openlibrary/plugins/openlibrary/home.py
index 50aeaa43b..c4c264c69 100644
--- a/openlibrary/plugins/openlibrary/home.py
+++ b/openlibrary/plugins/openlibrary/home.py
@@ -2,12 +2,13 @@
 
 import logging
 import random
+import threading
 
 import web
 
 from infogami import config  # noqa: F401 side effects may be needed
 from infogami.infobase.client import storify
-from infogami.utils import delegate
+from infogami.utils import delegate, features
 from infogami.utils.view import public, render_template
 from openlibrary.core import admin, cache, ia, lending
 from openlibrary.i18n import gettext as _
@@ -32,6 +33,12 @@ CAROUSELS_PRESETS = {
 
 
 def get_homepage(devmode):
+    debug = features.is_enabled('debug')
+    thread = (
+        'main' if threading.current_thread() == threading.main_thread() else 'other'
+    )
+    web.debug(f"DC_TEST: GET_HOMEPAGE {debug=} {thread=}")
+
     try:
         stats = admin.get_stats(use_mock_data=devmode)
     except Exception:
@@ -48,13 +55,20 @@ def get_homepage(devmode):
 
 
 def get_cached_homepage():
-    five_minutes = 5 * dateutil.MINUTE_SECS
+    five_minutes = 30
     lang = web.ctx.lang
     pd = web.cookies().get('pd', False)
     key = "home.homepage." + lang
     if pd:
         key += '.pd'
 
+    debug = features.is_enabled('debug')
+    # Check if main thread
+    thread = (
+        'main' if threading.current_thread() == threading.main_thread() else 'other'
+    )
+    web.debug(f"DC_TEST: GET_CACHED_HOMEPAGE {debug=} {thread=}")
+
     mc = cache.memcache_memoize(
         get_homepage, key, timeout=five_minutes, prethread=caching_prethread()
     )
diff --git a/openlibrary/templates/home/index.html b/openlibrary/templates/home/index.html
index e11428e18..1c814ec2a 100644
--- a/openlibrary/templates/home/index.html
+++ b/openlibrary/templates/home/index.html
@@ -16,6 +16,7 @@ $add_metatag(name="twitter:image:alt", content="Open Library Logo")
 $add_metatag(name="twitter:card", content="homepage_summary")
 
 <div id="contentBody">
+  <h1>DEBUG: $('debug' in ctx.features)</h1>
 
   $:render_template("home/welcome", test=test)

And now debug appears correctly as enabled, even when running off the main thread!

Note, I did spot another bug; when the cache is populated for the very first time and run on the main thread, it cached the debug value as True if that first request happened to have it set to true. This is a pre-existing issue, that likely causes a number of headaches with our cache. Created a PR to fix on OL: #10591

Also fixes https://sentry.archive.org/organizations/ia-ux/issues/482181/?environment=production&project=7&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=5

@cdrini cdrini mentioned this pull request Mar 17, 2025
@cdrini cdrini force-pushed the fix/features-missing-in-fakeload branch from abc5c77 to e82d6c6 Compare March 17, 2025 21:10
@cdrini cdrini marked this pull request as ready for review March 17, 2025 22:54
@cdrini
Copy link
Collaborator Author

cdrini commented Mar 18, 2025

Auto-merging this since it ended up being a very small change, and I tested both locally and patch deployed + Sentry monitored.

@cdrini cdrini merged commit 8196cd6 into master Mar 18, 2025
2 checks passed
@cdrini cdrini deleted the fix/features-missing-in-fakeload branch March 18, 2025 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate production ThreadedDict AttributeErrors
1 participant