Skip to content

ARCHBOM-1584: refactor monitoring app - #70

Merged
robrap merged 1 commit into
masterfrom
robrap/ARCHBOM-1584-add-monitoring-internal
Nov 2, 2020
Merged

ARCHBOM-1584: refactor monitoring app#70
robrap merged 1 commit into
masterfrom
robrap/ARCHBOM-1584-add-monitoring-internal

Conversation

@robrap

@robrap robrap commented Oct 30, 2020

Copy link
Copy Markdown
Contributor

Description:

  • Add ADR to explain new app organization.
  • Apply new app organization to the monitoring Django app.
  • Remove middleware ordering checks. This is not a typical Django feature and it is painful when refactoring.
  • Add CachedCustomMonitoringMiddleware, CodeOwnerMonitoringMiddleware, and MonitoringMemoryMiddleware to the public API.
  • Deprecate above middleware from old location.
  • Deprecate methods from modules that were always meant to be used from the public API:
    • accumulate
    • increment
    • set_custom_attribute
    • set_custom_attributes_for_course_key
  • Add additional custom attributes for deprecated classes and methods to make them safer to retire.

Note: Some additional details in the CHANGELOG.rst change in the PR.

JIRA:

ARCHBOM-1584

Testing instructions:

  • I will test with an edx-platform sandbox once edx-platform PR is ready.

Merge checklist:

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewer notes to help with the review. Please resolve comments that you are done with. Thank you.

Code coverage is an issue on some moved code that had no code coverage, like MonitoringMemoryMiddleware. I will try to add a smoke test.

Comment thread edx_django_utils/cache/middleware.py
Comment thread edx_django_utils/cache/tests/test_middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/tests/code_owner/test_utils.py

@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.

Looks great, ship it!

Comment thread edx_django_utils/monitoring/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/middleware.py Outdated
Comment thread edx_django_utils/monitoring/internal/middleware.py Outdated
Comment thread edx_django_utils/monitoring/internal/middleware.py Outdated
Comment thread edx_django_utils/monitoring/internal/middleware.py Outdated
@robrap

robrap commented Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for your thoughts, @idegtiarov. I'll look into whatever is reasonably quick.

  1. Black is hopefully coming in the future to help with code consistency.
  2. I was trying to squeeze in a step in the right direction, but there will always be more that can be done, especially for existing code that is being moved around.

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@idegtiarov: I did what made sense on this PR, and tried to add explanations.

I still have code coverage I need to add to land this PR.

Comment thread edx_django_utils/monitoring/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/code_owner/middleware.py
Comment thread edx_django_utils/monitoring/internal/middleware.py Outdated
@idegtiarov

Copy link
Copy Markdown

@robrap Thanks for the answers and clarifications.

* Add ADR to explain new app organization.
* Apply new app organization to the monitoring Django app.
* Remove middleware ordering checks. This is not a typical Django
feature and it is painful when refactoring.
* Add CachedCustomMonitoringMiddleware, CodeOwnerMonitoringMiddleware,
and MonitoringMemoryMiddleware to the public API.
* Deprecate above middleware from old location.
* Deprecate methods from modules that were always meant
to be used from the public API:
  * accumulate
  * increment
  * set_custom_attribute
  * set_custom_attributes_for_course_key
* Add additional custom attributes for deprecated
classes and methods to make them safer to retire.

ARCHBOM-1584
@robrap
robrap force-pushed the robrap/ARCHBOM-1584-add-monitoring-internal branch from 6da1a97 to 241bb42 Compare November 2, 2020 23:17
@robrap

robrap commented Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

@feanil: FYI: I plan on merging, but I wanted to let you know I added these tests for an existing middleware to increase coverage: https://github.com/edx/edx-django-utils/blob/241bb42ae5f511c25ab095018d05966cd509c63d/edx_django_utils/monitoring/tests/test_middleware.py

@robrap
robrap merged commit 8a2c205 into master Nov 2, 2020
@robrap
robrap deleted the robrap/ARCHBOM-1584-add-monitoring-internal branch November 2, 2020 23:22
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.

4 participants