Skip to content

feat: migrate to mitol-django-observability plugin#3346

Merged
feoh merged 4 commits intomainfrom
feat/mitol-django-observability
Mar 27, 2026
Merged

feat: migrate to mitol-django-observability plugin#3346
feoh merged 4 commits intomainfrom
feat/mitol-django-observability

Conversation

@blarghmatey
Copy link
Copy Markdown
Member

@blarghmatey blarghmatey commented Mar 2, 2026

  • Run uv lock to update the uv lock file
  • Commit and push the updated lock file to fix CI failures
  • Merge main into feature branch to resolve uv.lock conflict
  • Fix Sentry Issue 1: Bridge MITX_ONLINE_LOG_LEVELLOG_LEVEL env var so the observability plugin can consume it (backward compatibility)
  • Fix Sentry Issue 2: Restore AdminEmailHandler for django.request to preserve 500-error email notifications to admins in production

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Replace bespoke OpenTelemetry + logging boilerplate with the new
mitol-django-observability shared plugin.

Changes:
- Remove main/telemetry.py (configure_opentelemetry moved to plugin)
- Remove LOGGING dict from settings.py (provided by plugin)
- Add 'from mitol.observability.settings.logging import LOGGING'
- Add 'mitol.observability.apps.ObservabilityConfig' to INSTALLED_APPS
- Update pyproject.toml: add mitol-django-observability dep,
  remove direct opentelemetry-api/sdk/exporter deps

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 2, 2026

OpenAPI Changes

Show/hide ## Changes for v0.yaml:
## Changes for v0.yaml:


## Changes for v1.yaml:


## Changes for v2.yaml:


Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

Copy link
Copy Markdown
Contributor

@dsubak dsubak left a comment

Choose a reason for hiding this comment

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

Conceptually, this seems pretty straightforward. It'll change some logging behavior, but that's largely intentional afaict.

I'm going to hold off on formal approval until mitodl/ol-django#407 lands and I can actually start local with this branch. Since the package doesn't exist yet it just chokes on startup, which is totally expected but I do want to put it through the lightest testing ever before I put a stamp on it!

Comment thread pyproject.toml
"opentelemetry-api>=1.31.0",
"opentelemetry-exporter-otlp>=1.31.0",
"mitol-django-observability>=2026.1.0",
"opentelemetry-instrumentation-celery>=0.52b0",
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.

One like... super minor concern I've got w/ the reliance on automatic instrumentation is that we no longer get big obvious failures if we screw something up (like if I were to inadvertently remove any of these otel-instrumentation-* packages).

I wonder if there's any value in adding an "expected" set of automatic instrumentations over in the plugin that is be defined by settings and throwing an error if we ever fail that?

It feels like it's probably overkill at the moment to me, but if we ended up seeing people accidentally dropping instrumentation that they don't consult every day, it might be a relatively quick and cheap way to let folks avoid surprise viz gaps.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dsubak can you give more color to your concerns? Are you referring to the overall automated instrumentation that happens with the OTEL SDK, or are you referring to e.g. the Celery and Postgres instrumentation based on the discovery of dependencies?

Copy link
Copy Markdown
Contributor

@dsubak dsubak Mar 27, 2026

Choose a reason for hiding this comment

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

@blarghmatey The latter - previously, if I were to remove opentelemetry-instrumentation-celery things would explode loudly in main/telemetry.py because we have to import and explicitly configure those extensions.

Now I believe that auto_instrument will simply instrument whatever is installed (respecting the skip and allowlists), meaning removing one of those packages from this repo silently removes the instrumentation.

Like I said, I do think it's kind of a minor concern - the main edge case this would be getting ahead of is something like a well-meaning cleanup PR where since they can't find usages in the repo they assume it's good to be removed - but figured I'd bring it up for discussion in case folks felt it was more important than I do.

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.

@dsubak Always appreciative of/open to bar raising but I think this one's ready to merge given that you're saying this seems a minor concern. lmk if you have objections, and we can always revert.

@pdpinch
Copy link
Copy Markdown
Member

pdpinch commented Mar 27, 2026

@blarghmatey I think this is waiting on you now, so I assigned it back to you and added the "waiting on author" label

@feoh
Copy link
Copy Markdown
Contributor

feoh commented Mar 27, 2026

@copilot Please run 'uv lock' to update the uv lock file and commit/push that change to fix the failing tests.

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 27, 2026

@copilot Please run 'uv lock' to update the uv lock file and commit/push that change to fix the failing tests.

Done — ran uv lock and committed the updated lockfile in 735ce89. It resolved mitol-django-observability v2026.3.11, added structlog v25.5.0, and removed the now-redundant opentelemetry-exporter-otlp v1.39.1.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • releases.astral.sh
    • Triggering command: /home/REDACTED/.local/bin/uv /home/REDACTED/.local/bin/uv lock (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from feoh March 27, 2026 16:30
Copilot finished work on behalf of feoh March 27, 2026 16:30
@feoh
Copy link
Copy Markdown
Contributor

feoh commented Mar 27, 2026

@copilot pre-commit says there is a marge conflict. Please resolve that conflict if the correct resolution is clear and straight forward. Otherwise let me know and don't touch anything.

Comment thread main/settings.py Outdated
…lock

Co-authored-by: feoh <330070+feoh@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 27, 2026

@copilot pre-commit says there is a marge conflict. Please resolve that conflict if the correct resolution is clear and straight forward. Otherwise let me know and don't touch anything.

Resolved in 94943d1 — merged main into this branch (only uv.lock had a conflict since it's auto-generated), then re-ran uv lock to produce a clean lockfile that includes both the latest main changes and mitol-django-observability. No conflict markers remain.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • releases.astral.sh
    • Triggering command: /home/REDACTED/.local/bin/uv uv lock --project /home/REDACTED/work/mitxonline/mitxonline (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of feoh March 27, 2026 16:38
Comment thread main/settings.py Outdated
@feoh
Copy link
Copy Markdown
Contributor

feoh commented Mar 27, 2026

@copilot Please review the two issues Sentry reported and fix them if they are actual problems.

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 27, 2026

@copilot Please review the two issues Sentry reported and fix them if they are actual problems.

Both issues are real and have been fixed in 2f45f1a:

  1. MITX_ONLINE_LOG_LEVEL not consumed — the plugin reads LOG_LEVEL from os.environ directly, bypassing our get_string(name="MITX_ONLINE_LOG_LEVEL") definition. Fixed by adding os.environ.setdefault("LOG_LEVEL", LOG_LEVEL) before the plugin import, so the legacy env var is bridged through.

  2. AdminEmailHandler removed — the plugin's LOGGING config only has a console handler for django.request; no mail_admins handler. Fixed by extending the imported LOGGING dict to add back the require_debug_false filter, the AdminEmailHandler, and appending it to the django.request logger's handlers.

Copilot finished work on behalf of feoh March 27, 2026 16:57
@feoh feoh merged commit 8c2815f into main Mar 27, 2026
10 checks passed
@feoh feoh deleted the feat/mitol-django-observability branch March 27, 2026 19:38
This was referenced Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants