Repository navigation
feat(mit-ol): install real OTel packages in place of the inert opentelemetry-api - #177
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces inert OTel API dependencies across seven deployment cells with SDK, exporter, and framework instrumentors.
Changes:
- Adds OTel distro and OTLP HTTP exporter.
- Adds Django, Celery, and mysqlclient instrumentors.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Added a git override for `edx-django-utils` pinned to https://github.com/blarghmatey/edx-django-utils/tree/otel-create-span (the branch behind openedx/edx-django-utils#549) in all 7 cells, so this matrix also validates the actual `create_span()` fix rather than the no-op currently on PyPI. Local `check-deployment` for `mitxonline`/`master` still passes clean with the override in place. Drop it once #549 merges and releases. |
…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.
blarghmatey
force-pushed
the
otel-manifest-packages
branch
from
August 21, 2026 15:43
ea8e062 to
8d28a51
Compare
This was referenced Aug 21, 2026
Merged
blarghmatey
added a commit
to mitodl/ol-infrastructure
that referenced
this pull request
Aug 22, 2026
* fix(edxapp): remove dead uwsgi.ini ConfigMap and mounts 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). * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * feat(edxapp): wrap LMS/CMS/celery with opentelemetry-instrument 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. * Apply remaining changes Co-authored-by: blarghmatey <479088+blarghmatey@users.noreply.github.com> * fix(edxapp): move OTEL SDK env vars to container env and add command_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> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: blarghmatey <479088+blarghmatey@users.noreply.github.com>
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
opentelemetry-api-- installed, but nothing to instrument and no SDK/exporter, so it did nothing.opentelemetry-instrumentauto-instrumentation agent needs:opentelemetry-distro,opentelemetry-exporter-otlp-proto-http, and instrumentors for Django, Celery, and mysqlclient (confirmed edx-platform's actual DB driver --mysqlclient==2.2.8inrequirements/edx/base.txt,django.db.backends.mysqlthroughoutlms/envs/*/cms/envs/*-- not psycopg, which is what every other MIT service uses).Why open this now
A prior attempt at OTel here (ol-infrastructure#827, subtask #1948, 2024) hit a real protobuf version conflict against edx-platform's pin and needed a manual
pip uninstall protobuf && pip install --no-binary protobuf protobuf==4.25.1workaround plusPROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=pythonto run at all -- see the notes on mitodl/open-edx-plugins#213.Both sides of that conflict have moved since 2024 (opentelemetry-proto now declares
protobuf<8.0,>=5.0; edx-platform master currently pinsprotobuf==7.35.1), so it's very likely stale, and I confirmed a cleanuv pip compilelocally against master's exact pins. But that's a resolver check, not proof -- the original failure was a runtime crash (CMS reportedly crashed from "some kind of issue with timestamps coming out of the plugin"), which a resolver can't catch. This PR is how we find out for real:plugin-compatwill runcheck-deploymentagainst every cell, including the olderverawood/ulmorelease branches this local pass didn't cover.Ran
dagger call platform check-deployment --release-name master --deployment-name mitxonlinelocally before pushing: passed clean, 27/27 plugin distributions imported fine, full pip install succeeded with the new packages in place.Test plan
check-deploymentformaster/mitxonlinepasses.plugin-compatmatrix (triggered by this PR) passes for the remaining 6 cells, especiallyverawood/ulmowhere the 2024 conflict is more likely to reappear given their older edx-platform pins.