feat: add contextmanager-based management command observability filter - #344
feat: add contextmanager-based management command observability filter#344ktyagiapphelix2u wants to merge 7 commits into
Conversation
| @classmethod | ||
| def run_filter( | ||
| cls, | ||
| command_name: str, | ||
| service_variant: str, | ||
| command_runner: Callable[..., Any], | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
This signature is non-conventional. The input args (excluding cls) should map to an output tuple with identical parts:
| @classmethod | |
| def run_filter( | |
| cls, | |
| command_name: str, | |
| service_variant: str, | |
| command_runner: Callable[..., Any], | |
| ) -> dict[str, Any]: | |
| @classmethod | |
| def run_filter( | |
| cls, | |
| command_name: str, | |
| service_variant: str, | |
| command_runner: Callable[..., Any], | |
| ) -> tuple[str, str, Callable[..., Any]]: |
But also, see my comment on your platform PR: edx/edx-platform#200 (comment)
The revised signature would look like this instead:
| @classmethod | |
| def run_filter( | |
| cls, | |
| command_name: str, | |
| service_variant: str, | |
| command_runner: Callable[..., Any], | |
| ) -> dict[str, Any]: | |
| @classmethod | |
| def run_filter( | |
| cls, | |
| command_contextmanager: AbstractContextManager[None] | |
| command_name: str, | |
| service_variant: str, | |
| ) -> tuple[AbstractContextManager[None], str, str]: |
There was a problem hiding this comment.
I updated this filter to the context-manager pattern so inputs and outputs map positionally as a tuple. The signature is now command_contextmanager, command_name, service_variant -> (command_contextmanager, command_name, service_variant), and the test was updated to assert tuple unpacking accordingly. This also aligns with the platform-side change where manage.py remains the only execution callsite and the filter only wraps execution via a context manager.
feanil
left a comment
There was a problem hiding this comment.
The description currently describes what this is but it's unclear why we would want this filter? What's the reason to inject data into a management command at runtime instead of just adding more parameters to the management command to allow for variation? More context on the why of this filter would be useful.
Also, it would be nice to have some docs even if it's in the PR description of what it would look like to install this filter on a management command.
| org.openedx.platform.management.command.contextmanager.requested.v1 | ||
|
|
||
| Trigger: | ||
| - Repository: edx/edx-platform |
There was a problem hiding this comment.
| - Repository: edx/edx-platform | |
| - Repository: openedx/openedx-platform |
| self.assertEqual(command_contextmanager, filtered_command_contextmanager) | ||
| self.assertEqual(command_name, filtered_command_name) | ||
| self.assertEqual(service_variant, filtered_service_variant) |
There was a problem hiding this comment.
Use modern pytest-style assertions:
| self.assertEqual(command_contextmanager, filtered_command_contextmanager) | |
| self.assertEqual(command_name, filtered_command_name) | |
| self.assertEqual(service_variant, filtered_service_variant) | |
| assert command_contextmanager == filtered_command_contextmanager | |
| assert command_name == filtered_command_name | |
| assert service_variant == filtered_service_variant |
This filter isn't used to send extra options to management commands. Instead, it runs every management command in the same shared setup. Management commands don't go through Django's normal web request process, so the usual middleware doesn't run. This filter gives you one place to add things like logging, performance monitoring, error tracking, or Datadog tracing for all management commands, without having to change each command separately. |
da6330e to
75d43a7
Compare
Summary
This PR introduces the ManagementCommandContextmanagerRequested filter under the openedx_filters.management package.
The filter is invoked before a Django management command is executed, allowing plugins to inspect or modify the context manager, command name, and service variant through the standard Open edX filter pipeline. It follows the conventional filter contract where the inputs map directly to the returned tuple.
This PR also includes unit tests verifying the filter returns the expected values when no pipeline steps modify the inputs.
Changes
Added the openedx_filters.management package.
Added the ManagementCommandContextmanagerRequested public filter.
Added unit tests for the filter's default behavior.