Skip to content

Commit

Permalink
fixup! fix code review feedback
Browse files Browse the repository at this point in the history
1. switch get_code_owner_from_module => set_code_owner_attribute_from_module.
2. Update tests with deeper mock of set_custom_attribute.
3. Remove decorator set_code_owner_attribute and associated tests.
4. Fix minor rst issues.
  • Loading branch information
robrap committed Nov 14, 2024
1 parent d237378 commit 433efbf
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 75 deletions.
4 changes: 2 additions & 2 deletions edx_arch_experiments/datadog_monitoring/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from django.apps import AppConfig

from .code_owner.utils import get_code_owner_from_module
from .code_owner.utils import set_code_owner_attribute_from_module

log = logging.getLogger(__name__)

Expand All @@ -27,7 +27,7 @@ def on_span_start(self, span):
# We can use this for celery spans, because the resource name is more predictable
# and available from the start. For django requests, we'll instead continue to use
# django middleware for setting code owner.
get_code_owner_from_module(span.resource)
set_code_owner_attribute_from_module(span.resource)

def on_span_finish(self, span):
pass
Expand Down
29 changes: 0 additions & 29 deletions edx_arch_experiments/datadog_monitoring/code_owner/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"""
import logging
import re
from functools import wraps

from django.conf import settings
from edx_django_utils.monitoring import set_custom_attribute
Expand Down Expand Up @@ -148,34 +147,6 @@ def set_code_owner_custom_attributes(code_owner):
set_custom_attribute('code_owner_2_squad', squad)


def set_code_owner_attribute(wrapped_function):
"""
Decorator to set the code_owner_2 and code_owner_2_module custom attributes.
Celery tasks or other non-web functions do not use middleware, so we need
an alternative way to set the code_owner_2 custom attribute.
Usage::
@task()
@set_code_owner_2_attribute
def example_task():
...
Note: If the decorator can't be used for some reason, call
``set_code_owner_attribute_from_module`` directly.
An untested potential alternative is documented in the ADR covering this decision:
docs/decisions/0003-code-owner-for-celery-tasks.rst
"""
@wraps(wrapped_function)
def new_function(*args, **kwargs):
set_code_owner_attribute_from_module(wrapped_function.__module__)
return wrapped_function(*args, **kwargs)
return new_function


def clear_cached_mappings():
"""
Clears the cached code owner mappings. Useful for testing.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Understanding code owner custom attributes

If you first need some background on the ``code_owner_2_squad`` and ``code_owner_2_theme`` custom attributes, see `Using Code_Owner Custom Span Tags`_.

.. _Using Code_Owner Custom Span Tags: https://github.com/openedx/edx-arch-experiments/blob/master/edx_arch_experiments/datadog_monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst
.. _Using Code_Owner Custom Span Tags: https://github.com/edx/edx-arch-experiments/blob/main/edx_arch_experiments/datadog_monitoring/docs/how_tos/add_code_owner_custom_attribute_to_an_ida.rst

Expand and contract name changes
--------------------------------
Expand All @@ -24,7 +24,7 @@ Example expand phase::
code_owner_2_squad:('old-squad-name', 'new-squad-name')
code_owner_2_theme:('old-theme-name', 'new-theme-name')

Example contract phase NRQL::
Example contract phase::

code_owner_2_squad:'new-squad-name'
code_owner_2_theme:'new-theme-name'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,10 @@
from edx_arch_experiments.datadog_monitoring.code_owner.utils import (
clear_cached_mappings,
get_code_owner_from_module,
set_code_owner_attribute,
set_code_owner_attribute_from_module,
)


@set_code_owner_attribute
def decorated_function(pass_through):
"""
For testing the set_code_owner_attribute decorator.
"""
return pass_through


@ddt.ddt
class MonitoringUtilsTests(TestCase):
"""
Expand Down Expand Up @@ -92,30 +83,6 @@ def test_mapping_performance(self):
average_time = time / call_iterations
self.assertLess(average_time, 0.0005, f'Mapping takes {average_time}s which is too slow.')

@override_settings(
CODE_OWNER_MAPPINGS={'team-red': ['edx_arch_experiments.datadog_monitoring.tests.code_owner.test_utils']},
CODE_OWNER_THEMES={'team': ['team-red']},
)
@patch('edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute')
def test_set_code_owner_attribute_success(self, mock_set_custom_attribute):
self.assertEqual(decorated_function('test'), 'test')
self._assert_set_custom_attribute(
mock_set_custom_attribute, code_owner='team-red', module=__name__, check_theme_and_squad=True
)

@override_settings(
CODE_OWNER_MAPPINGS={'team-red': ['edx_arch_experiments.datadog_monitoring.tests.code_owner.test_utils']},
CODE_OWNER_THEMES='invalid-setting',
)
def test_set_code_owner_attribute_with_invalid_setting(self):
with self.assertRaises(TypeError):
decorated_function('test')

@patch('edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute')
def test_set_code_owner_attribute_no_mappings(self, mock_set_custom_attribute):
self.assertEqual(decorated_function('test'), 'test')
self._assert_set_custom_attribute(mock_set_custom_attribute, code_owner=None, module=__name__)

@override_settings(CODE_OWNER_MAPPINGS={
'team-red': ['edx_arch_experiments.datadog_monitoring.tests.code_owner.test_utils']
})
Expand Down
18 changes: 9 additions & 9 deletions edx_arch_experiments/datadog_monitoring/tests/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,32 +43,32 @@ def get_processor_list():
class TestDatadogMonitoringSpanProcessor(TestCase):
"""Tests for DatadogMonitoringSpanProcessor."""

@patch('edx_arch_experiments.datadog_monitoring.apps.get_code_owner_from_module')
def test_celery_span(self, mock_get_code_owner):
@patch('edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute')
def test_celery_span(self, mock_set_custom_attribute):
""" Tests processor with a celery span. """
proc = apps.DatadogMonitoringSpanProcessor()
celery_span = FakeSpan('celery.run', 'test.module.for.celery.task')

proc.on_span_start(celery_span)

mock_get_code_owner.assert_called_once_with('test.module.for.celery.task')
mock_set_custom_attribute.assert_called_once_with('code_owner_2_module', 'test.module.for.celery.task')

@patch('edx_arch_experiments.datadog_monitoring.apps.get_code_owner_from_module')
def test_other_span(self, mock_get_code_owner):
@patch('edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute')
def test_other_span(self, mock_set_custom_attribute):
""" Tests processor with a non-celery span. """
proc = apps.DatadogMonitoringSpanProcessor()
celery_span = FakeSpan('other.span', 'test.resource.name')

proc.on_span_start(celery_span)

mock_get_code_owner.assert_not_called()
mock_set_custom_attribute.assert_not_called()

@patch('edx_arch_experiments.datadog_monitoring.apps.get_code_owner_from_module')
def test_non_span(self, mock_get_code_owner):
@patch('edx_arch_experiments.datadog_monitoring.code_owner.utils.set_custom_attribute')
def test_non_span(self, mock_set_custom_attribute):
""" Tests processor with an object that doesn't have span name or resource. """
proc = apps.DatadogMonitoringSpanProcessor()
non_span = object()

proc.on_span_start(non_span)

mock_get_code_owner.assert_not_called()
mock_set_custom_attribute.assert_not_called()

0 comments on commit 433efbf

Please sign in to comment.