-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Course assets should be served by a view rather than a middleware #34702
Comments
See #34702 This necessarily involves switching from calling `StaticContent.is_versioned_asset_path` to determine whether to handle the request to having a hardcoded urlpattern. I've made the choice to hardcode the other two patterns similarly rather than using imported constants. The mapping of URL patterns to database records should be explicit (even though we don't expect those constants to change out from under us.) I've renamed the middleware rather than choosing a new name for the implementation because there are other references in tests and other code. This was the smaller change. A note on HTTP methods: The middleware currently completely ignores the request's HTTP method, so I wanted to confirm that only GETs were being used in practice. This query reveals that 99.8% of requests that this middleware handles are GET, with just a smattering of PROPFIND and OPTIONS and a tiny number of HEAD and POST: ``` from Transaction select count(*) facet request.method where name = 'WebTransaction/Function/openedx.core.djangoapps.contentserver.middleware:StaticContentServer' since 4 weeks ago ```
See #34702 This necessarily involves switching from calling `StaticContent.is_versioned_asset_path` to determine whether to handle the request to having a hardcoded urlpattern. I've made the choice to hardcode the other two patterns similarly rather than using imported constants. The mapping of URL patterns to database records should be explicit (even though we don't expect those constants to change out from under us.) I've renamed the middleware rather than choosing a new name for the implementation because there are other references in tests and other code. This was the smaller change. A note on HTTP methods: The middleware currently completely ignores the request's HTTP method, so I wanted to confirm that only GETs were being used in practice. This query reveals that 99.8% of requests that this middleware handles are GET, with just a smattering of PROPFIND and OPTIONS and a tiny number of HEAD and POST: ``` from Transaction select count(*) facet request.method where name = 'WebTransaction/Function/openedx.core.djangoapps.contentserver.middleware:StaticContentServer' since 4 weeks ago ```
+1. This seriously confused Connor and I today! |
…ew (#34703) See #34702 This necessarily involves switching from calling `StaticContent.is_versioned_asset_path` to determine whether to handle the request to having a hardcoded urlpattern. I've made the choice to hardcode the other two patterns similarly rather than using imported constants. The mapping of URL patterns to database records should be explicit (even though we don't expect those constants to change out from under us.) I've renamed the middleware rather than choosing a new name for the implementation because there are other references in tests and other code. This was the smaller change. A note on HTTP methods: The middleware currently completely ignores the request's HTTP method, so I wanted to confirm that only GETs were being used in practice. This query reveals that 99.8% of requests that this middleware handles are GET, with just a smattering of PROPFIND and OPTIONS and a tiny number of HEAD and POST: ``` from Transaction select count(*) facet request.method where name = 'WebTransaction/Function/openedx.core.djangoapps.contentserver.middleware:StaticContentServer' since 4 weeks ago ```
Private 2U dashboard for monitoring cutover: https://app.datadoghq.com/dashboard/vjs-j3j-czr |
We don't actually own this, and it is no longer a high priority, but we might be able to help with rollout. |
@timmc-edx is this work complete, or is there anything left to do? |
We're still in the early stage of this work. I had been hoping to try the cutover once the Datadog traces were fixed, but that's not happening any time soon, so maybe we should do that sooner instead. |
Nice to have, but close to done, so we just want to close this out. |
Looks like I enabled this for stage.edx.org on June 10. Haven't seen any complaints, and the graphs look fine. |
Enabled on edge.edx.org but had to disable again as I discovered that it was causing almost all c4x URLs to return 404. Not sure why yet. EDIT: Example for testing: |
@timmc-edx: This is a P4 that we made a P3 because we were hoping to simply ship an implemented solution. Now that a bug was found, we should probably timebox a resolution, rather than just plowing through this no matter how long it takes. |
Agreed, although I think the fix will be pretty straightforward. |
This was failing to capture /c4x/ URLs when the `content_server.use_view` waffle flag was enabled, so we were returning 404s for those. Part of #34702
This was failing to capture /c4x/ URLs when the `content_server.use_view` waffle flag was enabled, so we were returning 404s for those during our rollout test. Part of #34702
This was failing to capture /c4x/ URLs when the `content_server.use_view` waffle flag was enabled, so we were returning 404s for those during our rollout test. Part of #34702
This was failing to capture /c4x/ URLs when the `content_server.use_view` waffle flag was enabled, so we were returning 404s for those during our rollout test. Part of #34702
edx.org has been converted over successfully. We'll let this bake for a week or two in production and then we can move the implementation properly into views.py and remove the old middleware. |
@timmc-edx: FYI: I moving this out of blocked because it has sufficiently baked based on your last comment. |
…flag) (#35420) Final planned portion of #34702 -- waffle flag and middleware are removed. Commits: 1. Feature change - Delete `content_server.use_view` waffle flag in favor of always using view - Delete the husk of `StaticContentServerMiddleware` and references to it - Update views module docstring 2. Refactor (move) - Move contentserver implementation into views.py 3. Lint cleanup - Fix import ordering (from refactor debris + amnestied lint)
…flag) (#35420) Final planned portion of #34702 -- waffle flag and middleware are removed. Commits: 1. Feature change - Delete `content_server.use_view` waffle flag in favor of always using view - Delete the husk of `StaticContentServerMiddleware` and references to it - Update views module docstring 2. Refactor (move) - Move contentserver implementation into views.py 3. Lint cleanup - Fix import ordering (from refactor debris + amnestied lint)
This is largely complete, and now just wants a bit of optional cleanup. Acceptance criteria updated. |
Some lingering cleanup from the transition from a middleware to a view. See #34702 for context. - Remove IMPL and self/StaticContentServer references - Add newlines to satisfy code style linter - Fix test references - Update module docstring
Some lingering cleanup from the transition from a middleware to a view. See #34702 for context. - Remove IMPL and self/StaticContentServer references - Add newlines to satisfy code style linter - Fix test references - Update module docstring
Closing out -- last bit of cleanup would involve turning this into three views, and probably isn't worth the effort right now. |
Acceptance criteria:
IMPL
variable andStaticContentServer
class by lifting methods to module scopeCourse asset URLs like
/asset-v1:...
are served from a middleware atopenedx.core.djangoapps.contentserver.middleware.StaticContentServer
rather than from a view. This means that Django starts handling the request as a Not Found (because there is no matching urlpattern), but then the middleware intercepts the request and returns a response of its own. There wasn't any good reason for doing this instead of using a view, as far as I can tell. It causes some problems for telemetry: When the code-owner middleware asks Django what view handled the request, it does so by looking at the result of theresolve
utility, but these URLs get a Resolver404 (because there's no registered urlpattern).Implementation suggestion:
PRs:
The text was updated successfully, but these errors were encountered: