fix: Add management command monitoring pipeline - #16
Conversation
67e7bd0 to
02ad9fb
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new Open edX Filters pipeline step to wrap Django management command execution with Datadog observability, gated by a new waffle flag, and introduces tests and tooling updates to support the new behavior.
Changes:
- Introduces
ManagementCommandMonitoringPipelineStepand amonitor_management_command()context manager for Datadog transaction naming and custom attributes. - Adds a new waffle flag (
filters_pipelines.enable_management_command_monitoring) to enable/disable monitoring. - Expands tests for the new pipeline step and adds
pytest-mockto test dependencies; updates tox coverage target module name.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Updates pytest coverage target to the importable package name (edx_filters_pipelines). |
| tests/test_edx-filters-pipelines.py | Adds unit tests for management command monitoring step behavior (enabled/disabled/custom trace name). |
| requirements/test.in | Adds pytest-mock as a test dependency input. |
| requirements/test.txt | Updates compiled test requirements to include pytest-mock. |
| edx_filters_pipelines/waffle.py | Adds ENABLE_MANAGEMENT_COMMAND_MONITORING waffle flag definition and metadata. |
| edx_filters_pipelines/management/pipelines/monitoring.py | Adds the monitoring pipeline step and Datadog-wrapping context manager. |
| edx_filters_pipelines/management/init.py | Introduces package init for management integrations. |
| edx_filters_pipelines/management/pipelines/init.py | Introduces package init for management pipelines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
requirements/test.txt:200
pytest-mockis added to the compiledrequirements/test.txtwithout a pinned version, while the rest of this file is version-pinned. This breaks reproducible installs and likely indicatespip-compile(or equivalent) wasn’t run after updatingrequirements/test.in.
pytest-mock
# via -r requirements/test.in
edx_filters_pipelines/waffle.py:28
- The new waffle flag definition introduces a tab-indented line inside the
WaffleFlag(...)call. This is inconsistent with the rest of the file (spaces-only) and can trip linting/formatting.
ENABLE_MANAGEMENT_COMMAND_MONITORING = WaffleFlag(
f'{WAFFLE_NAMESPACE}.enable_management_command_monitoring', __name__
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
edx_filters_pipelines/management/pipelines/monitoring.py:68
- The intent here appears to be “best-effort” monitoring (log and continue without monitoring), but only the toggle check/context creation is protected. If
monitor_management_command().__enter__()or__exit__()raises (e.g., Datadog API issues while setting attributes or duration), the management command itself will fail. Consider entering/exiting the monitoring context defensively so monitoring failures never prevent the command from running or completing, while still propagatingcommand_runnerexceptions.
try:
if ENABLE_MANAGEMENT_COMMAND_MONITORING.is_enabled():
monitor_context = monitor_management_command(command_name, service_variant, trace_name)
except Exception: # pylint: disable=broad-except
log.exception(
edx_filters_pipelines/waffle.py:22
- The PR description mentions gating via a waffle flag under the
filters_pipelines.*namespace, but this implementation is aSettingTogglenamedFILTERS_PIPELINES_ENABLE_MANAGEMENT_COMMAND_MONITORING. Please ensure the PR description and downstream wiring/docs match the actual toggle mechanism and name (or switch to aWaffleFlagif that’s what consumers expect).
# .. toggle_name: FILTERS_PIPELINES_ENABLE_MANAGEMENT_COMMAND_MONITORING
# .. toggle_implementation: SettingToggle
# .. toggle_default: False
# .. toggle_description: Settings toggle to enable Datadog monitoring for Django management commands.
# .. toggle_use_cases: opt_in
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
edx_filters_pipelines/management/pipelines/monitoring.py:74
- The try/except only guards toggle lookup and context-manager construction; if Datadog setup inside monitor_management_command raises during enter (e.g., set_custom_attribute / set_monitoring_transaction_name), the exception will propagate and the management command will not run, despite the log message claiming it will continue without monitoring. Consider entering the monitoring context in a guarded way so monitoring failures don’t block command execution.
try:
if ENABLE_MANAGEMENT_COMMAND_MONITORING.is_enabled():
monitor_context = monitor_management_command(command_name, service_variant, trace_name)
except Exception: # pylint: disable=broad-except
log.exception(
'Failed to initialize management command monitoring for %s; continuing without monitoring.',
command_name,
)
with monitor_context:
return command_runner()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
edx_filters_pipelines/management/pipelines/monitoring.py:116
ManagementCommandMonitoringPipelineStepdoesn’t document how it’s intended to be wired intoOPEN_EDX_FILTERS_CONFIG(filter name, step path) or how to enable it via the new SettingToggle. The otherPipelineStepimplementations in this repo include configuration guidance, and this one introduces a newoperation_nameextra_config option.
class ManagementCommandMonitoringPipelineStep(PipelineStep):
"""
Add monitoring around Django management command execution.
"""
edx_filters_pipelines/management/pipelines/monitoring.py:138
monitor_management_command’soperation_nameis passed positionally fromrun_filter. Using a keyword argument here avoids accidental argument reordering if the helper signature changes and makes the call site clearer.
monitor_contextmanager = monitor_management_command(
command_name,
service_variant,
operation_name,
)
tests/test_edx-filters-pipelines.py:284
- The exception-handling parametrized test gate (
exception_class_recorded) only assertsexception_messagewhen exception metadata is expected, but does not assert thatmanagement_command.exception_classis set. This leaves a regression hole (e.g., exception_class recording could be removed without failing tests).
if exception_class_recorded:
set_custom_attribute.assert_any_call('management_command.exception_message', str(raised_exception))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
edx_filters_pipelines/management/pipelines/monitoring.py:57
- The repo currently pins
edx-django-utils==8.0.0(e.g.,requirements/base.txt:42), but this code callsfunction_trace(..., operation_name=...). Per the PR description,operation_namesupport requiresedx-django-utils>=8.1.0, so this will raise aTypeErrorat runtime unless the dependency pins are updated accordingly (or a backwards-compatible fallback is added).
start_time = time.monotonic()
status = 'failure'
try:
with function_trace(resource_name, operation_name=operation_name):
yield
There was a problem hiding this comment.
In base.in, you could add a minimum requirement with a comment based on: "Requires edx-django-utils 8.1.0+ for operation_name support.".
There was a problem hiding this comment.
I checked this and agree with the intent, but CI currently cannot resolve edx-django-utils>=8.1.0 from the available package index (latest visible there is 8.0.1), so adding that minimum in base.in causes install failures. I have not added the 8.1.0 floor for now to keep the pipeline green.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
edx_filters_pipelines/management/pipelines/monitoring.py:147
- The
try/excepthere only wraps construction ofmonitor_management_command(...), but not entering it. Since@contextmanagercode (includingfunction_trace.__enter__) runs on__enter__, failures during enter will still prevent the command from running, contradicting the log message "continuing without monitoring". Enter the monitoring context inside the guarded block (e.g., viaExitStack.enter_context) so initialization failures truly fail open.
try:
if ENABLE_MANAGEMENT_COMMAND_MONITORING.is_enabled():
monitor_contextmanager = monitor_management_command(
command_name,
service_variant,
edx_filters_pipelines/management/pipelines/monitoring.py:56
function_trace(..., operation_name=...)relies onoperation_namesupport inedx-django-utils, but the repository currently pinsedx-django-utils==8.0.0(e.g.,requirements/base.txt:42). With that pin, this call can raiseTypeError: function_trace() got an unexpected keyword argument 'operation_name'at runtime. Please bump the pinnededx-django-utilsversion to 8.1.0+ (per PR description) in the compiled requirements files.
with function_trace(resource_name, operation_name=operation_name):
Summary
Adds optional Datadog monitoring for Django management commands via edx_filters_pipelines.
Changes
Jira ticket
https://2u-internal.atlassian.net/browse/BOMS-151