feat!: switch code owner middleware to signals #866
Conversation
| if 'request' in kwargs: | ||
| set_code_owner_attribute(kwargs['request']) |
There was a problem hiding this comment.
It's probably worth just documenting the provided kwargs in edx-django-utils so we can put them directly in the receiver function's signature.
There was a problem hiding this comment.
- I added directly to the signature, but I left the defensive code, since this is for monitoring. Thoughts?
- Can you explain where you want to see the kwargs documented? Do you mean here https://github.com/openedx/edx-django-utils/blob/master/edx_django_utils/monitoring/README.rst#monitoring-support-middleware-and-monitoring-plugins? Other? Do you think this is an important change, or is it close enough to simply allow someone to search for the implementation?
There was a problem hiding this comment.
I don't think the defensive code is needed, since e-d-u is using send_robust.
I'd say document the kwargs where ever you've got other documentation -- either in the rST there or in docstrings. In my opinion it would be best for it to be as close to the code as possible, so that changes to the code are more likely to involve changes to docs/comments, but rST is probably fine too, since that's already where you describe the role and usage of the signals.
Initial rollout of moving code_owner monitoring code from edx-django-utils proved that the new CodeOwnerMonitoringMiddleware was not added high enough in the middleware stack. Instead, we have added signals to edx-django-utils's MonitoringSupportMiddleware, and this datadog_monitoring plugin now listens to those signals in order to automatically add code_owner custom span tags for django requests. BREAKING CHANGE: Removes CodeOwnerMonitoringMiddleware. See #784
- add min constraint edx-django-utils>=7.1.0 - change changelog date and fix typo - * add unreleased changelog to 6.0.0 - add request argument
robrap
force-pushed
the
robrap/code-owner-middleware-to-signals
branch
from
cf5a688 to
57ef2d5
Compare
|
Note: I confirmed that all uses of |
Initial rollout of moving code_owner monitoring code from edx-django-utils proved that the new
CodeOwnerMonitoringMiddleware was not added high enough in the middleware stack. Instead, we have added signals to edx-django-utils's MonitoringSupportMiddleware, and this datadog_monitoring plugin now listens to those signals in order to automatically add code_owner custom span tags for django requests.
BREAKING CHANGE: Removes CodeOwnerMonitoringMiddleware.
See #784
Merge checklist:
Check off if complete or not applicable: