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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 19 commits into from
Nov 21, 2024
Merged

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

merged 19 commits into from
Nov 21, 2024

Conversation

robrap
Copy link
Contributor

@robrap robrap commented Oct 23, 2024 •
edited
Loading

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

robrap
robrap commented Oct 23, 2024
View reviewed changes
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.

robrap
robrap commented Nov 13, 2024
View reviewed changes
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.

edx_arch_experiments/datadog_monitoring/apps.py Outdated Show resolved Hide resolved
edx_arch_experiments/datadog_monitoring/code_owner/utils.py Outdated Show resolved Hide resolved
...experiments/datadog_monitoring/docs/how_tos/update_monitoring_for_squad_or_theme_changes.rst Outdated Show resolved Hide resolved
...experiments/datadog_monitoring/docs/how_tos/update_monitoring_for_squad_or_theme_changes.rst Outdated Show resolved Hide resolved
edx_arch_experiments/datadog_monitoring/tests/test_app.py Outdated Show resolved Hide resolved
robrap and others added 5 commits November 14, 2024 13:44
@robrap
feat: original copy of code_owner code
32aa239 
This is the code_owner code as found in edx-django-utils.
@robrap
feat: refactor code_owner code from edx-dajango-utils
5cbbf48 
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
@timmc-edx @robrap
fixup! Re-compile requirements for 3.11 on Linux (without --upgrade)
68e019d
@robrap
fixup! fix some quality failures
d237378
@robrap
fixup! fix code review feedback
433efbf 
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
timmc-edx
timmc-edx approved these changes Nov 14, 2024
View reviewed changes
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.

@robrap robrap mentioned this pull request Nov 20, 2024
Open
2 tasks
timmc-edx
timmc-edx reviewed Nov 21, 2024
View reviewed changes
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.

Hmm, though I had saved up comments for a review, but I guess they submitted already? Anyway, here's a last one (which I accidentally left on a commit but then moved here...)

edx_arch_experiments/datadog_monitoring/tests/test_apps.py Show resolved Hide resolved
@timmc-edx
Copy link
Member

(None of those are blocking comments.)

robrap and others added 2 commits November 21, 2024 12:55
@robrap @timmc-edx
fixup! minor update
678bae3 
Co-authored-by: Tim McCormack <[email protected]>
@robrap @timmc-edx
fixup! minor update
9e14b61 
Co-authored-by: Tim McCormack <[email protected]>
@robrap robrap merged commit ee70bda into main Nov 21, 2024
6 checks passed
@robrap robrap deleted the robrap/add-code-owner-monitoring branch November 21, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timmc-edx timmc-edx timmc-edx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robrap @timmc-edx @edx-requirements-bot