Skip to content

feat: add contextmanager-based management command observability filter - #344

Open
ktyagiapphelix2u wants to merge 7 commits into
openedx:mainfrom
ktyagiapphelix2u:ktyagi/filter
Open

feat: add contextmanager-based management command observability filter#344
ktyagiapphelix2u wants to merge 7 commits into
openedx:mainfrom
ktyagiapphelix2u:ktyagi/filter

Conversation

@ktyagiapphelix2u

@ktyagiapphelix2u ktyagiapphelix2u commented Mar 27, 2026

Copy link
Copy Markdown

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.

@ktyagiapphelix2u
ktyagiapphelix2u marked this pull request as ready for review July 14, 2026 13:37
Comment thread openedx_filters/management/filters.py Outdated
Comment on lines +30 to +36
@classmethod
def run_filter(
cls,
command_name: str,
service_variant: str,
command_runner: Callable[..., Any],
) -> dict[str, Any]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signature is non-conventional. The input args (excluding cls) should map to an output tuple with identical parts:

Suggested change
@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:

Suggested change
@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]:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ktyagiapphelix2u ktyagiapphelix2u changed the title feat: add ManagementCommandExecutionRequested filter for observability feat: add contextmanager-based management command observability filter Jul 28, 2026

@feanil feanil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread openedx_filters/management/filters.py Outdated
org.openedx.platform.management.command.contextmanager.requested.v1

Trigger:
- Repository: edx/edx-platform

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Repository: edx/edx-platform
- Repository: openedx/openedx-platform

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@pwnage101 pwnage101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more things:

  • Address Feanil's comment by providing a more detailed docstring about the expected use case.
  • One minor test issue (see below).
  • Bump the version.
  • squash all commits into 1.

Comment on lines +37 to +39
self.assertEqual(command_contextmanager, filtered_command_contextmanager)
self.assertEqual(command_name, filtered_command_name)
self.assertEqual(service_variant, filtered_service_variant)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use modern pytest-style assertions:

Suggested change
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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@ktyagiapphelix2u

Copy link
Copy Markdown
Author

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants