Skip to content

Commit

Permalink
feat: Remove background_task and hide get_current_transaction.
Browse files Browse the repository at this point in the history
background_task is seemingly unusued, so it has been removed.

Also add comments explaining get_current_transaction is
internal-only and will not be exposed via API. Remove most
public references to it. Also refactor code so that it
is only used in the file where it's used.
  • Loading branch information
dianakhuang committed Oct 17, 2024
1 parent 2d7519c commit c8d5a19
Show file tree
Hide file tree
Showing 8 changed files with 50 additions and 76 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ Change Log

.. There should always be an "Unreleased" section for changes pending release.
7.0.0 - 2024-10-16
------------------
Removed
~~~~~~~
* Remove unused ``background_task`` monitoring function.
* Remove ``get_current_transaction`` (used internally only) from the public API.

[6.1.0] - 2024-10-15
---------------------
Changed
Expand Down
2 changes: 1 addition & 1 deletion edx_django_utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
EdX utilities for Django Application development..
"""

__version__ = "6.1.0"
__version__ = "7.0.0"

default_app_config = (
"edx_django_utils.apps.EdxDjangoUtilsConfig"
Expand Down
6 changes: 1 addition & 5 deletions edx_django_utils/monitoring/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,14 @@ Feature support matrix for built-in telemetry backends:
- ✅
- ❌
- ✅
* - Retrieve and manipulate spans (``get_current_transaction``, ``ignore_transaction``)
* - Manipulate spans (``ignore_transaction``)
- ✅
- ❌
- ❌
* - Record exceptions (``record_exception``)
- ✅
- ✅
- ✅
* - Instrument non-web tasks (``background_task``)
- ✅
- ❌
- ❌

Additional requirements for using these backends:

Expand Down
3 changes: 1 addition & 2 deletions edx_django_utils/monitoring/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,9 @@
MonitoringMemoryMiddleware,
MonitoringSupportMiddleware
)
from .internal.transactions import get_current_transaction, ignore_transaction
from .internal.transactions import ignore_transaction
from .internal.utils import (
accumulate,
background_task,
function_trace,
increment,
record_exception,
Expand Down
40 changes: 39 additions & 1 deletion edx_django_utils/monitoring/internal/code_owner/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from django.urls import resolve
from django.urls.exceptions import Resolver404

from ..transactions import get_current_transaction
from ..utils import set_custom_attribute
from .utils import (
_get_catch_all_code_owner,
Expand All @@ -15,9 +14,48 @@
set_code_owner_custom_attributes
)

try:
import newrelic.agent
except ImportError:
newrelic = None # pylint: disable=invalid-name

Check warning on line 20 in edx_django_utils/monitoring/internal/code_owner/middleware.py

View check run for this annotation

Codecov / codecov/patch

edx_django_utils/monitoring/internal/code_owner/middleware.py#L19-L20

Added lines #L19 - L20 were not covered by tests

log = logging.getLogger(__name__)


class MonitoringTransaction():
"""
Represents a monitoring transaction (likely the current transaction).
"""
def __init__(self, transaction):
self.transaction = transaction

@property
def name(self):
"""
The name of the transaction.
For NewRelic, the name may look like:
openedx.core.djangoapps.contentserver.middleware:StaticContentServer
"""
if self.transaction and hasattr(self.transaction, 'name'):
return self.transaction.name
return None


def get_current_transaction():
"""
Returns the current transaction. This is only used internally and won't
be ported over to the backends framework, because transactions will be
very different based on the backend.
"""
current_transaction = None
if newrelic:
current_transaction = newrelic.agent.current_transaction()

return MonitoringTransaction(current_transaction)


class CodeOwnerMonitoringMiddleware:
"""
Django middleware object to set custom attributes for the owner of each view.
Expand Down
32 changes: 0 additions & 32 deletions edx_django_utils/monitoring/internal/transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,35 +28,3 @@ def ignore_transaction():
# an equivalent for Datadog. For Datadog, use filter/ignore rules.
if newrelic: # pragma: no cover
newrelic.agent.ignore_transaction()


class MonitoringTransaction():
"""
Represents a monitoring transaction (likely the current transaction).
"""
def __init__(self, transaction):
self.transaction = transaction

@property
def name(self):
"""
The name of the transaction.
For NewRelic, the name may look like:
openedx.core.djangoapps.contentserver.middleware:StaticContentServer
"""
if self.transaction and hasattr(self.transaction, 'name'):
return self.transaction.name
return None


def get_current_transaction():
"""
Returns the current transaction.
"""
current_transaction = None
if newrelic:
current_transaction = newrelic.agent.current_transaction()

return MonitoringTransaction(current_transaction)
11 changes: 1 addition & 10 deletions edx_django_utils/monitoring/tests/test_custom_monitoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from django.test import TestCase, override_settings

from edx_django_utils.cache import RequestCache
from edx_django_utils.monitoring import MonitoringSupportMiddleware, accumulate, get_current_transaction, increment
from edx_django_utils.monitoring import MonitoringSupportMiddleware, accumulate, increment

from ..middleware import CachedCustomMonitoringMiddleware as DeprecatedCachedCustomMonitoringMiddleware
from ..middleware import MonitoringCustomMetricsMiddleware as DeprecatedMonitoringCustomMetricsMiddleware
Expand Down Expand Up @@ -128,15 +128,6 @@ def test_error_tagging(self, mock_get_root_span):
type(fake_exception), fake_exception, fake_exception.__traceback__
)

@patch('newrelic.agent')
def test_get_current_transaction(self, mock_newrelic_agent):
mock_newrelic_agent.current_transaction().name = 'fake-transaction'
current_transaction = get_current_transaction()
self.assertEqual(current_transaction.name, 'fake-transaction')

def test_get_current_transaction_without_newrelic(self):
current_transaction = get_current_transaction()
self.assertEqual(current_transaction.name, None)

@patch('edx_django_utils.monitoring.utils.internal_accumulate')
def test_deprecated_accumulate(self, mock_accumulate):
Expand Down
25 changes: 0 additions & 25 deletions edx_django_utils/monitoring/tests/test_utils.py

This file was deleted.

0 comments on commit c8d5a19

Please sign in to comment.