Repository navigation
feat(edxapp): wrap LMS/CMS/celery with opentelemetry-instrument - #5560
Merged
Merged
Conversation
edxapp switched to Granian (import_uwsgi_config=False, granian_config=GranianConfig(...) already in place) but the uwsgi.ini ConfigMap, its volume/mount on LMS and CMS, and the UWSGI_WORKERS env var were never removed. Nothing reads uwsgi.ini anymore -- confirmed no other reference to uwsgi anywhere in this repo's edx-platform/lehrer-adjacent code. Also simplifies the celery volume-mount filter, which existed only to exclude uwsgi.ini from celery containers; with uwsgi.ini gone there's nothing left to filter. Verified with `pulumi preview` against mitxonline.CI: clean diff, uwsgi-ini ConfigMap deleted, LMS/CMS/celery Deployments get a normal rolling update, the two pre-deploy Jobs replace (expected -- Job pod specs are immutable, same as any other pod-spec change to them).
for more information, see https://pre-commit.ci
Installing OTel packages in the image (mitodl/lehrer#177) doesn't activate anything on its own: LMS/CMS run under granian and workers under a plain celery invocation, and edx_django_utils.monitoring. OpenTelemetryBackend only decorates whatever span is already active -- it never installs a TracerProvider itself. Per copilot review on lehrer#177, wrap every process with the opentelemetry-instrument auto-instrumentation agent so there's an actual SDK+exporter for it to decorate. Adds command_prefix to OLApplicationK8sConfig (empty by default, so every other consumer of granian_config is unaffected) and sets it for edxapp's LMS/CMS only. Celery/beat/process_scheduled_emails already set their container command directly, so those just get prefixed in place. Two things confirmed by actually running opentelemetry-instrument against opentelemetry-distro + opentelemetry-exporter-otlp-proto-http (the exact package set lehrer#177 installs), not assumed from docs: - OTEL_TRACES_EXPORTER defaults to "otlp", which resolves against an entry point named "otlp_proto_http" here (only the HTTP exporter is installed, not grpc) -- "otlp" alone doesn't match, so it must be set explicitly or configuration fails. - More importantly: OTEL_METRICS_EXPORTER and OTEL_LOGS_EXPORTER also default to "otlp" (grpc, not installed), and the SDK resolves traces/metrics/logs exporters together in one call that raises on the FIRST one it can't find -- so an unset metrics/logs exporter aborts trace configuration too, not just metrics. Confirmed via RuntimeError: "Requested component 'otlp_proto_grpc' not found in entry point 'opentelemetry_metrics_exporter'". Worse, this failure is completely silent in the normal wrapped-process path -- opentelemetry-instrument's sitecustomize swallows the exception by default -- so without this fix the wrap would produce zero traces with no error anywhere. Set OTEL_METRICS_EXPORTER/OTEL_LOGS_EXPORTER to "none" since neither signal is part of this rollout. Also moves OTEL_SERVICE_NAME out of the shared interpolated-config env dict (env_name + "-edxapp" for every workload, which would have collapsed LMS/CMS/all five celery deployments into one Tempo service) into each workload's own env, distinct per deployment. ORDERING HAZARD: opentelemetry-instrument must actually be installed in the edxapp image before this deploys, or granian/celery fail to start (missing executable). Do not merge/deploy ahead of mitodl/lehrer#177 shipping. Verified with `pulumi preview` against mitxonline.CI: all five celery-family containers and both LMS/CMS granian containers show command prefixed with opentelemetry-instrument, OTEL_SERVICE_NAME distinct per workload, no unexpected resource changes.
1 of 2 tasks
blarghmatey
added a commit
to mitodl/lehrer
that referenced
this pull request
Aug 21, 2026
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
added a commit
to mitodl/lehrer
that referenced
this pull request
Aug 21, 2026
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.
Ardiea
approved these changes
Aug 21, 2026
blarghmatey
added a commit
to mitodl/lehrer
that referenced
this pull request
Aug 22, 2026
…180) * feat(mit-ol): install real OTel packages in place of the inert opentelemetry-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. * feat(mit-ol): pull edx-django-utils create_span() fix via git override 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. * feat(local-dev): wrap lms/cms/workers with opentelemetry-instrument 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. * fix(settings): resolve Derived settings sentinels order-independently 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 * fix(local-dev): create edxapp_csmh with correct name and migrate it 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 --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: blarghmatey <479088+blarghmatey@users.noreply.github.com>
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Adds OpenTelemetry auto-instrumentation to edxapp web and Celery-family workloads.
Changes:
- Adds reusable command-prefix support.
- Wraps LMS, CMS, and five worker processes with
opentelemetry-instrument. - Assigns workload-specific service names and exporter settings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
components/services/k8s.py |
Adds command-prefix handling. |
applications/edxapp/k8s_resources.py |
Instruments edxapp workloads and assigns service names. |
applications/edxapp/k8s_configmaps.py |
Adds OpenTelemetry exporter configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # to a gRPC exporter that isn't installed. Traces resolving to a missing | ||
| # exporter fails silently (opentelemetry-instrument swallows the exception | ||
| # by default) -- explicit here rather than relying on the default. | ||
| "OTEL_TRACES_EXPORTER": "otlp_proto_http", |
| effective_cmd_array = ol_app_k8s_config.application_cmd_array | ||
| effective_arg_array = ol_app_k8s_config.application_arg_array | ||
|
|
||
| if ol_app_k8s_config.command_prefix and effective_cmd_array is not None: |
Member
Author
|
@copilot address the pr feedback, move the otel settings into the container environment |
Co-authored-by: blarghmatey <479088+blarghmatey@users.noreply.github.com>
…prefix tests - Remove OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_TRACES_EXPORTER, OTEL_METRICS_EXPORTER, OTEL_LOGS_EXPORTER, OTEL_LOG_LEVEL from k8s_configmaps.py (Django YAML config). opentelemetry-instrument reads the process environment before Django loads YAML, so these settings had no effect there. - Add _OTEL_SDK_ENV module-level dict in k8s_resources.py with those five vars. Spread into application_config for LMS and CMS (renders as container EnvVarArgs), and into the env list for all 5 hand-rolled celery deployments (lms-celery, lms-high-mem-celery, lms-beat, process-scheduled-emails, cms-celery). - Add 3 Pulumi-mock unit tests for command_prefix in test_k8s_extra_containers.py: default Granian command unchanged, prefix prepended to Granian command, prefix prepended to explicit application_cmd_array. Co-authored-by: blarghmatey <479088+blarghmatey@users.noreply.github.com>
Contributor
Done in commit |
blarghmatey
added a commit
to mitodl/lehrer
that referenced
this pull request
Aug 24, 2026
…181) build-ulmo-xpro-edxapp-image has been failing since ~Aug 21 with ModuleNotFoundError: No module named 'django_aqueduct' from lms/envs/aqueduct.py's `from django_aqueduct import configure_django_settings` -- that settings module (deployments/mit-ol/ settings/lms/aqueduct.py) is injected by the lehrer Dagger build into every cell, but the ulmo/xpro cell's packages list never had django-aqueduct in it. Every sibling cell (master/mitxonline, master/mitx, master/mitx-staging, verawood/mitx, verawood/mitx-staging, verawood/xpro) already lists django-aqueduct==0.13.0 in the identical position, right after pydantic-settings[yaml]; ulmo/xpro simply omitted it. Downstream effect on the ol-infrastructure side: because no fresh ulmo image has built successfully since before edxapp's LMS/CMS/celery opentelemetry-instrument wrapping landed (mitodl/ol-infrastructure#5560), xpro-qa's edxapp deploy has been stuck redeploying its last successful (stale, pre-opentelemetry) ulmo image, whose containers now crashloop with `exec: "opentelemetry-instrument": executable file not found in $PATH`. That should self-resolve once this lands and a fresh ulmo image builds. Verified: schema validation passes (jsonschema against build_manifest.schema.json) and pre-commit is clean. Attempted `lehrer build check --cell mit-ol/ulmo/xpro` (the same boot-check the failing Concourse job runs) but it couldn't complete in this sandbox -- Dagger's engine failed to resolve python:3.11-bookworm from Docker Hub (DNS timeout on auth.docker.io), a sandbox network restriction unrelated to this change. The fix is a single-line addition that exactly mirrors six already-working sibling cells in the same file.
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.
What are the relevant tickets?
N/A -- addresses a copilot review comment on mitodl/lehrer#177.
Description (What does it do?)
Stacked on #5557 (dead uwsgi cleanup) since both touch the same LMS/CMS config dicts.
lehrer#177 installs OTel packages in the edxapp image, but installing them doesn't activate anything: LMS/CMS run under
granianand workers under a plainceleryinvocation, andedx_django_utils.monitoring.OpenTelemetryBackendonly decorates whatever span is already active -- it never installs a TracerProvider itself. Copilot flagged exactly this on lehrer#177. This PR wraps every process with theopentelemetry-instrumentauto-instrumentation agent.command_prefix: list[str]toOLApplicationK8sConfigin the sharedk8s.pycomponent -- empty by default, so every othergranian_configconsumer (mit_learn, mitxonline, etc.) is unaffected. Set to["opentelemetry-instrument"]for edxapp's LMS/CMS only.OTEL_SERVICE_NAMEout of the shared interpolated-config env dict (env_name + "-edxapp"for every workload, which collapsed LMS/CMS/all five celery deployments into one Tempo service) into each workload's own env, distinct per deployment.Two things I confirmed by actually running
opentelemetry-instrumentagainstopentelemetry-distro+opentelemetry-exporter-otlp-proto-httplocally (the exact package set lehrer#177 installs), not assumed from docs:OTEL_TRACES_EXPORTERdefaults to"otlp", which needs an entry point named exactly"otlp"-- only"otlp_proto_http"is registered here (grpc isn't installed), so it must be set explicitly or the exporter fails to resolve.OTEL_METRICS_EXPORTER/OTEL_LOGS_EXPORTERalso default to"otlp"(grpc), and the SDK resolves traces/metrics/logs exporters together in one call that raises on the first one it can't find -- so an unset metrics/logs exporter aborts trace configuration too. Confirmed viaRuntimeError: Requested component 'otlp_proto_grpc' not found in entry point 'opentelemetry_metrics_exporter'. Worse: this failure is completely silent in the actual wrapped-process path --opentelemetry-instrument's sitecustomize swallows the exception by default -- so without this fix the wrap would produce zero traces with no error anywhere. Set both to"none"since neither signal is part of this rollout.How can this be tested?
Ran
pulumi preview --diffagainstmitxonline.CI. All 5 celery-family containers and both LMS/CMS granian containers showcommandprefixed withopentelemetry-instrument;OTEL_SERVICE_NAMEis distinct per workload; no unexpected resource changes beyond the normal rolling updates.Additional Context
Ordering hazard: do not merge/deploy ahead of mitodl/lehrer#177 shipping and a new edxapp image being built with it.
opentelemetry-instrumentmust actually be present in the image or granian/celery fail to start (missing executable).Per the project's staged-rollout guidance, consider deploying to QA and a low-volume workload first rather than flipping LMS/CMS in production immediately -- LMS/CMS are the highest-volume workloads in the cluster, and an under-sized tail sampler previously caused a ~98% trace-drop incident earlier this project.