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

feat: refactor code_owner code from edx-dajango-utils #838

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

robrap
Copy link
Contributor

@robrap robrap commented Oct 23, 2024

Note to Reviewer: The original edx-django-utils code was copied in as a separate commit to make it easier to see what was added and what was updated if you review commit by commit.

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

Merge checklist:
Check off if complete or not applicable:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

requirements/base.txt Outdated Show resolved Hide resolved
@robrap robrap force-pushed the robrap/add-code-owner-monitoring branch from ce0a75e to 1d3af1c Compare October 24, 2024 14:52
@robrap
Copy link
Contributor Author

robrap commented Oct 24, 2024

Note to Reviewer: I did initial local testing to POC the celery span resource name was available locally, but I did not do a final local test. I wasn't able to get to a real celery task locally.

Copy link
Contributor Author

@robrap robrap left a comment

Choose a reason for hiding this comment

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

@timmc-edx: Thanks. This is ready for re-review. It needs to be rebased, but that requires re-running make upgrade, and I still don't know how to do that cleanly. It would be great to have a way to do so.

robrap and others added 5 commits November 14, 2024 13:44
This is the code_owner code as found in edx-django-utils.
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
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.
@robrap robrap force-pushed the robrap/add-code-owner-monitoring branch from 8137b68 to 433efbf Compare November 14, 2024 18:56
Copy link
Member

@timmc-edx timmc-edx left a comment

Choose a reason for hiding this comment

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

Looks good! Reminder to update date in changelog.

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.

2 participants