Repository navigation
feat(local-dev): wrap lms/cms/workers with opentelemetry-instrument - #180
Merged
Merged
Conversation
…lemetry-api Every cell listed a bare opentelemetry-api with nothing to instrument anything and no SDK/exporter, so it did nothing. Replace it with the package set the opentelemetry-instrument auto-instrumentation agent needs: opentelemetry-distro (api+sdk+instrumentation core), opentelemetry-exporter-otlp-proto-http, and instrumentors for Django, Celery, and mysqlclient (edx-platform's actual DB driver, not psycopg). A prior attempt at OTel here (ol-infrastructure#827/#1948, 2024) hit a protobuf version conflict against edx-platform's pin and needed a manual downgrade + pure-Python protobuf fallback to work around it. Local `check-deployment` against mitxonline/master passed clean with today's pins; this PR is also how we find out whether the older verawood/ulmo release branches still hit that conflict.
OpenTelemetryBackend.create_span() is a no-op on PyPI today, so function_trace() silently skips span creation for every OTel-backed call site. Fixed upstream in openedx/edx-django-utils#549, not yet merged/released -- override to install straight from that PR branch so the plugin-compat matrix (and anything built off this manifest) gets the real behavior instead of the broken PyPI release. Drop this override once #549 merges and a release ships.
Contributor
There was a problem hiding this comment.
Pull request overview
Activates OpenTelemetry auto-instrumentation for local LMS, CMS, and worker processes.
Changes:
- Wraps four runtime commands with
opentelemetry-instrument. - Configures console tracing and distinct service names.
- Adds required OpenTelemetry packages to the generic image manifest.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
deployment-worker.yaml |
Instruments the LMS worker. |
deployment-lms.yaml |
Instruments the LMS web process. |
deployment-cms.yaml |
Instruments the CMS web process. |
deployment-cms-worker.yaml |
Instruments the CMS worker. |
configmap-lms.yaml |
Configures LMS/worker exporters. |
configmap-cms.yaml |
Configures CMS/worker exporters. |
build_manifest.yaml |
Adds OpenTelemetry dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
blarghmatey
force-pushed
the
lehrer-local-dev-otel-wrap
branch
from
August 21, 2026 16:55
e9bb58c to
88cf9b3
Compare
Same fix as lehrer#177's follow-up in ol-infrastructure (mitodl/ol-infrastructure#5560): installing the OTel packages alone doesn't activate anything, since edx_django_utils.monitoring. OpenTelemetryBackend never installs a TracerProvider itself -- the process needs to actually run under opentelemetry-instrument. local-dev builds from deployments/generic/build_manifest.yaml by default (Tiltfile's DEPLOYMENT default is "generic"), not deployments/mit-ol/, which lehrer#177 didn't touch -- so opentelemetry-instrument wasn't even installed here. Added the same package set there. Defaults to the console exporter (OTEL_TRACES_EXPORTER=console) since there's no OTel collector running in local dev -- traces show up directly in `kubectl logs`/Tilt's log pane rather than needing Tempo locally. Same OTEL_METRICS_EXPORTER/OTEL_LOGS_EXPORTER=none reasoning as ol-infrastructure#5560: those default to a gRPC exporter that isn't installed, and an unresolvable metrics/logs exporter aborts the whole SDK configuration, traces included, silently. OTEL_SERVICE_NAME is set per-deployment (lms/cms/lms-worker/ cms-worker) rather than in the shared configmaps, since lms and lms-worker share lms-config (same for cms) and a shared value would collapse both into one service name. Verified: `dagger call platform check-deployment` for the generic cell installs the new packages and imports cleanly (exit 0). Did not have a live k3d cluster to verify a real trace appears in a pod's console log -- worth a spot-check on `lehrer dev start` before merging.
blarghmatey
force-pushed
the
lehrer-local-dev-otel-wrap
branch
from
August 21, 2026 16:57
88cf9b3 to
4fe8f86
Compare
configure_django_settings() collects base module attributes via dir() (alphabetical), which breaks Derived() dependency chains that don't happen to sort in declaration order (e.g. FRONTEND_REGISTER_URL depends on LMS_ROOT_URL, which sorts after it). Left unresolved, the Derived sentinel fails Django's tuple_settings validation on LOCALE_PATHS; that exception gets silently swallowed by ManagementUtility.execute(), and a corrupted reentrant settings snapshot (from the nested Settings.__init__ triggered by an XBlock's eager translation check) sticks permanently instead of the correct one. resolve_derived_settings() retries per-key against the final merged module until all Derived() values converge, independent of collection order. Also makes _derive_service_root_urls always populate LMS_ROOT_URL/CMS_ROOT_URL (falling back to the devstack default when LMS_BASE_URL/CMS_BASE_URL aren't set), which the boot-check gate's minimal environment needs to pass Django's common_initialization.E001 check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7WeAQ9q7X1gprEQfripva
The Database CR's metadata.name (edxapp-csmh) was used as the literal MySQL database name since spec.name wasn't set, so mariadb-operator created "edxapp-csmh" (hyphen) instead of "edxapp_csmh" (underscore) — the name Django's DATABASES config expects. LMS/CMS heartbeat then failed with "Unknown database 'edxapp_csmh'". Separately, the migrate Job only ran against the default connection; coursewarehistoryextended (student_module_history) is a distinct database that migrate never targets unless --database is passed explicitly, so its tables were never created even once the database existed under the right name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G7WeAQ9q7X1gprEQfripva
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
edx_django_utils.monitoring.OpenTelemetryBackendnever installs a TracerProvider itself -- the process needs to actually run underopentelemetry-instrument.command:on all 4 local-dev deployments (lms,cms,lms-worker,cms-worker) withopentelemetry-instrument.local-devbuilds fromdeployments/generic/build_manifest.yamlby default (Tiltfile'sDEPLOYMENTdefault isgeneric), notdeployments/mit-ol/, which feat(mit-ol): install real OTel packages in place of the inert opentelemetry-api #177 didn't touch -- soopentelemetry-instrumentwasn't even installed in the local-dev image. Added the same package set (opentelemetry-distro,opentelemetry-exporter-otlp-proto-http, Django/Celery/mysqlclient instrumentors) there.consoleexporter (OTEL_TRACES_EXPORTER=console) since there's no OTel collector running in local dev -- traces show up directly inkubectl logs/Tilt's log pane.OTEL_METRICS_EXPORTER/OTEL_LOGS_EXPORTER=nonefor the same reason as the ol-infrastructure companion PR: those default to a gRPC exporter that isn't installed, and an unresolvable metrics/logs exporter aborts the whole SDK configuration (traces included), silently.OTEL_SERVICE_NAMEset per-deployment rather than in the shared configmaps, sincelms/lms-workersharelms-config(same forcms/cms-worker) and a shared value would collapse both into one service name.Companion PR in ol-infrastructure doing the same thing for production/QA: mitodl/ol-infrastructure#5560.
Test plan
dagger call platform check-deployment --release-name master --deployment-name generic: passes clean, new packages install and the full generic package set imports without conflict.lehrer dev start-- worth a spot-check before merging.