Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEPR and move code owner monitoring code with updates #784

Closed
8 of 9 tasks
robrap opened this issue Aug 28, 2024 · 9 comments
Closed
8 of 9 tasks

DEPR and move code owner monitoring code with updates #784

robrap opened this issue Aug 28, 2024 · 9 comments
Assignees

Comments

@robrap
Copy link
Contributor

robrap commented Aug 28, 2024

  • DEPR of code owner code should happen because this is likely used by 2U-only, so it is just the right thing to do.
  • Also, having 2U-only-code would allow us to refactor more easily:
    • We could drop theme. (e.g.code_owner and code_owner_squad would have the same value, and code_owner_theme would be dropped. In theory, we could also drop code_owner_squad, but that might force too much unnecessary work on other teams.
    • We could refactor how celery tasks get code_owner without having to support anything other than Datadog:

Implementation thoughts:

  • We should experiment with a replacement solution before creating a DEPR ticket, so we know if we have any blockers before we sign ourselves up.
  • CodeOwnerMonitoringMiddleware could no longer be added directly into edx-platform here.
    • We could try using edx-platform's mechanism for adding additional middleware, assuming it doesn't run too late. Note: we could experimentally set a temporary span tag and see how many spans get code_owner, but don't get our new test tag.
    • If this didn't work out, we could consider introducing an openedx-event from DeploymentMonitoringMiddleware that allows operators to add their own custom monitoring.
  • Could be a separate ticket, but we used to have a New Relic search script for alerts and dashboards. It would be nice to have the same for DD for this type of work, and other monitoring refactors that have come up, like service name changes.

ACs:

  • Code has been moved from edx-django-utils to edx-arch-experiments plugin, so it is no longer public.
  • Theme functionality is removed from the codebase, as it is no longer valuable.
  • DEPR created for Open edX removal.

Implementation Tasks:

  • Confirm 6.1.0 with phase 2 code owner is working.
  • Confirm TNL has removed their code_owner reference from the CMS dashboard. See Slack thread.
    • Clean-up Studio references to CODE_OWNER_MAPPINGS.
  • Clean-up LMS references to CODE_OWNER_MAPPINGS.
  • Remove temporary span tag code_owner_plugin
  • Delete Datadog dashboard for DEPR.
@robrap robrap added this to Arch-BOM Aug 28, 2024
@robrap robrap moved this to Backlog in Arch-BOM Aug 28, 2024
@robrap
Copy link
Contributor Author

robrap commented Aug 28, 2024

Giving this a P3 because it is important, but it is not urgent.

@jristau1984 jristau1984 moved this from Backlog to Ready For Development in Arch-BOM Sep 9, 2024
@robrap robrap self-assigned this Oct 16, 2024
robrap added a commit that referenced this issue Oct 23, 2024
Initial rollout of moving code_owner monitoring code from
edx-django-utils to this plugin.

- Adds near duplicate of code owner middleware from
  edx-django-utils.
- Adds code owner for celery using Datadog span processing
  of celery.run spans.
- Uses temporary span tags names using `_2`, like
  `code_owner_2`, for rollout and comparison with the original span tags.

See #784
@jristau1984 jristau1984 moved this from In Progress to Blocked in Arch-BOM Oct 28, 2024
@robrap robrap moved this from Blocked to In Progress in Arch-BOM Nov 12, 2024
@robrap
Copy link
Contributor Author

robrap commented Nov 12, 2024

Presumably this was marked blocked while I was out. Marking in-progress again.

robrap added a commit that referenced this issue Nov 14, 2024
Initial rollout of moving code_owner monitoring code from
edx-django-utils to this plugin.

- Adds near duplicate of code owner middleware from
  edx-django-utils.
- Adds code owner for celery using Datadog span processing
  of celery.run spans.
- Uses temporary span tags names using `_2`, like
  `code_owner_2`, for rollout and comparison with the original span tags.

See #784
@robrap
Copy link
Contributor Author

robrap commented Nov 20, 2024

Phase 1:

  • Roll out new _2 monitoring from feat: refactor code_owner code from edx-dajango-utils #838
  • Check DD to see if _2 tags seem to match the existing span tags
    Phase 2:
  • Add ability to disable code-owner code from edx-django-utils by fixing bug related to reading settings.
  • Determine what we want going forward. Options.
    • Keep existing tags.
    • Selected option: Remove theme, and re-use but change data in code_owner tag,
    • Remove theme and come up with a new span tag name.
  • Load new tag off separate settings, so we can enable new code and disable old code separately.
  • Update jobs to also write new settings.
  • Possibly create DD search script if we want to change code_owner, and see if it is in use.
    • If it is not in use, and we want to change it, we may want to add a separate tag to disable it from the old code while leaving the rest, for simpler expand/contract.

robrap added a commit that referenced this issue Nov 21, 2024
Initial rollout of moving code_owner monitoring code from
edx-django-utils to this plugin.

- Adds near duplicate of code owner middleware from
  edx-django-utils.
- Adds code owner span tags for celery using Datadog span
  processing of celery.run spans.
- Uses temporary span tags names using `_2`, like `code_owner_2`,
  for rollout and comparison with the original span tags.
- Span tag code_owner_2_module includes the task name, where
  the original code_owner_module does not. In both cases, the
  code owner is computed the same, because it is based on a prefix match.

See #784
@robrap
Copy link
Contributor Author

robrap commented Dec 2, 2024

  • Created a dashboard to help compare legacy and new span tags.
  • Missing code_owner_2 for some errors and OPTIONS. May be due to tagging in a later middleware. Needs additional investigation. More notes on dashboard.
  • The good news is that celery.run spans seem to have the code owner using the new method of instrumentation. Could use a little more proof. Maybe this works even better?

robrap added a commit that referenced this issue Dec 4, 2024
Initial rollout of moving code_owner monitoring code from
edx-django-utils proved that the new
CodeOwnerMonitoringMiddleware was not added high enough
in the middleware stack. Instead, we have added signals
to edx-django-utils's MonitoringSupportMiddleware, and
this datadog_monitoring plugin now listens to those
signals in order to automatically add code_owner custom
span tags for django requests.

BREAKING CHANGE: Removes CodeOwnerMonitoringMiddleware.

See #784
robrap added a commit that referenced this issue Dec 5, 2024
This implements a TODO comment stating that once
edx-platform import_shims is no longer used, we
could remove the _OPTIONAL_MODULE_PREFIX_PATTERN
implementation.
See https://github.com/openedx/edx-platform/tree/854502b560bda74ef898501bb2a95ce238cf794c/import_shims

Phase 1 of the code owner refactor is an opportune time
to make this clean-up because we can compare code_owner and
code_owner_2 results.

See #784
robrap added a commit that referenced this issue Dec 5, 2024
This implements a TODO comment stating that once
edx-platform import_shims is no longer used, we
could remove the _OPTIONAL_MODULE_PREFIX_PATTERN
implementation.
See https://github.com/openedx/edx-platform/tree/854502b560bda74ef898501bb2a95ce238cf794c/import_shims

Phase 1 of the code owner refactor is an opportune time
to make this clean-up because we can compare code_owner and
code_owner_2 results.

See #784
robrap added a commit that referenced this issue Dec 5, 2024
Initial rollout of moving code_owner monitoring code from
edx-django-utils proved that the new
CodeOwnerMonitoringMiddleware was not added high enough
in the middleware stack. Instead, we have added signals
to edx-django-utils's MonitoringSupportMiddleware, and
this datadog_monitoring plugin now listens to those
signals in order to automatically add code_owner custom
span tags for django requests.

BREAKING CHANGE: Removes CodeOwnerMonitoringMiddleware.

See #784
robrap added a commit that referenced this issue Dec 5, 2024
Initial rollout of moving code_owner monitoring code from
edx-django-utils proved that the new
CodeOwnerMonitoringMiddleware was not added high enough
in the middleware stack. Instead, we have added signals
to edx-django-utils's MonitoringSupportMiddleware, and
this datadog_monitoring plugin now listens to those
signals in order to automatically add code_owner custom
span tags for django requests.

BREAKING CHANGE: Removes CodeOwnerMonitoringMiddleware.

See #784
@jristau1984 jristau1984 moved this from In Review to In Progress in Arch-BOM Dec 9, 2024
robrap added a commit that referenced this issue Dec 10, 2024
Completes code owner monitoring updates, which drops owner
theme and finalizes the code owner span tags. See doc and
ADR updates for more details.

* The code_owner_theme_2 tag was dropped altogether.
* The temporary suffix (_2) was removed from other span
  tags.
* The code_owner (formerly code_owner_2) tag no longer
  includes the theme name.
* The new name for the django setting is
  CODE_OWNER_TO_PATH_MAPPINGS (formerly
  CODE_OWNER_MAPPINGS).
* The django setting CODE_OWNER_THEMES was dropped.
* Updates the generate_code_owner_mappings.py script
  accordingly.

Implements:
- #784
robrap added a commit that referenced this issue Dec 10, 2024
Completes code owner monitoring updates, which drops owner
theme and finalizes the code owner span tags. See doc and
ADR updates for more details.

* The code_owner_theme_2 tag was dropped altogether.
* The temporary suffix (_2) was removed from other span
  tags.
* The code_owner (formerly code_owner_2) tag no longer
  includes the theme name.
* The new name for the django setting is
  CODE_OWNER_TO_PATH_MAPPINGS (formerly
  CODE_OWNER_MAPPINGS).
* The django setting CODE_OWNER_THEMES was dropped.
* Updates the generate_code_owner_mappings.py script
  accordingly.

Implements:
- #784
@robrap
Copy link
Contributor Author

robrap commented Dec 10, 2024

Created DEPR, but it still needs to be announced.

robrap added a commit that referenced this issue Dec 12, 2024
The span tag code_owner_plugin was used to rollout
code_owner switch from edx-django-utils to the
datadog_monitoring plugin in this repo. Now that
rollout is complete, this span tag can be removed.

Implements:
- #784
robrap added a commit that referenced this issue Dec 12, 2024
The span tag code_owner_plugin was used to rollout
code_owner switch from edx-django-utils to the
datadog_monitoring plugin in this repo. Now that
rollout is complete, this span tag can be removed.

Implements:
- #784
@robrap
Copy link
Contributor Author

robrap commented Dec 12, 2024

Here is the actual DEPR:

@robrap
Copy link
Contributor Author

robrap commented Dec 12, 2024

This work is complete. The actual DEPR work will be left to the DEPR ticket, and may become a 2U ticket if I decide we should take that on to really complete this.

@robrap robrap closed this as completed Dec 12, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Arch-BOM Dec 12, 2024
@robrap robrap moved this from Done to In Progress in Arch-BOM Dec 12, 2024
@robrap
Copy link
Contributor Author

robrap commented Dec 12, 2024

  • Although I made some doc updates, I need to do a final pass and update some additional docs.

@robrap robrap moved this from In Progress to Done in Arch-BOM Dec 13, 2024
@jristau1984 jristau1984 moved this from Done to Done - Long Term Storage in Arch-BOM Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done - Long Term Storage
Development

No branches or pull requests

1 participant