fix: implement OpenTelemetryBackend.create_span() - #549
Merged
Conversation
create_span() was a no-op (returned None), so every function_trace() call site silently skipped span creation for the OpenTelemetry backend -- unlike NewRelicBackend and DatadogBackend, which both return a real span/context manager from their create_span(). Return tracer.start_as_current_span(name) instead, matching the shape of the other two backends.
|
Thanks for the pull request, @blarghmatey! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
blarghmatey
added a commit
to mitodl/lehrer
that referenced
this pull request
Aug 21, 2026
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.
Merged
2 tasks
blarghmatey
added a commit
to mitodl/lehrer
that referenced
this pull request
Aug 21, 2026
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
added a commit
to mitodl/lehrer
that referenced
this pull request
Aug 22, 2026
…lemetry-api (#177) * 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.
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>
feoh
approved these changes
Aug 24, 2026
feoh
approved these changes
Aug 25, 2026
robrap
approved these changes
Aug 25, 2026
robrap
left a comment
robrap
left a comment
Contributor
There was a problem hiding this comment.
I probably would have called this 8.1.0 rather than 8.0.2, but not worth being concerned about. Just noting in case more changes come in. Thanks.
Contributor
|
@blarghmatey: Do you need/want me to merge? |
Contributor
Author
|
If you can that would be great, also feel free to do the 8.1.0 bump first if you prefer, or I canake that change |
Contributor
|
@blarghmatey; I’ll merge/release tomorrow. Version change is optional, but I don’t plan to update. I’ll merge what I find tomorrow. Feel free to update or leave as-is. |
Contributor
blarghmatey
added a commit
to mitodl/lehrer
that referenced
this pull request
Sep 1, 2026
… fork (#208) * fix(mit-ol): install forum from our branch, drop the edx-django-utils fork Two override changes, in opposite directions. Forum: the Typesense search backend is non-functional on every search, not just some -- per_page is set above Typesense's hard cap of 250, and the topic filter names a field the collection schema does not declare. Both have been there since the backend shipped. Fix is openedx/forum#289, which is green and awaiting review; until it merges and releases, the master cells install forum from the branch. Scoped to master only: our branch is based on forum master, so pointing verawood or ulmo at it would jump them across release boundaries for no benefit -- both still have forum search on Elasticsearch. This changes nothing at runtime today, since typesense:forum_search_enabled is false everywhere. It is what makes flipping that toggle possible. edx-django-utils: openedx/edx-django-utils#549 merged and shipped in 8.0.2, so the fork branch is no longer needed. edx-platform master already pins 8.0.2 and the three master cells simply drop the override. The verawood and ulmo cells cannot: those branches pin 8.0.1, which predates the fix, so they take an explicit 8.0.2 pin instead. Deleting the override outright would have quietly regressed OpenTelemetryBackend.create_span() on four of the seven cells. * fix(mit-ol): extend the forum override to verawood I had scoped this to the master cells out of caution rather than evidence. Having actually diffed 0.4.2 against forum master, verawood can take it too. The delta is ten commits, and only three modules are genuinely removed once the src/ layout move is normalised away: backends/mongodb/*, forum_create_mongodb_indexes, and toggles.py. None of that reaches us. All twelve environments already run forum_v2.enable_mysql_backend for everyone, so nobody is on the Mongo data backend. Nothing in openedx-platform imports forum.backends.mongodb or forum.toggles, and nothing in ol-infrastructure or lehrer invokes the removed management command. toggles.py only ever defined the enable_mysql_backend waffle flag itself, which is redundant now that MySQL is the sole backend -- worth noting that our set_waffle_flags entries for it become dead config. The rest is packaging (uv, src layout, semantic-release) and typing.cast() wrappers around request.data in the views, which are no-ops at runtime. ulmo stays on 0.4.1, now for a concrete reason rather than caution: forum master declares requires-python >=3.12 and the ulmo cell builds on 3.11, so it cannot install. verawood builds on 3.12, and forum master's only other constraint is Django>=4.2. Verified by installing the branch into a clean 3.12 venv: builds, reports 0.4.5, and the installed forum/search/typesense.py carries TYPESENSE_MAX_PER_PAGE = 250 and the singular commentable_id filter.
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
5 participants
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.
Description:
OpenTelemetryBackend.create_span()is currently a no-op (pass, implicitly returnsNone).function_trace()inmonitoring/internal/utils.pyhandles that by simply skipping the context manager when a backend'screate_span()returnsNone:So with
OPENEDX_TELEMETRY = ['edx_django_utils.monitoring.OpenTelemetryBackend'], everyfunction_trace()call site across a consuming service silently does nothing for OTel -- no span is created, no error is raised. BothNewRelicBackend.create_span()andDatadogBackend.create_span()already return real spans/context managers.This PR implements it the same way, using
tracer.start_as_current_span(name).JIRA:
N/A
Dependencies:
None.
Testing instructions:
OpenTelemetryBackend.create_span()was previously untested against any real behavior -- the existing OTel test coverage intest_backends.pypatchesNonRecordingSpan.set_attribute/record_exceptiondirectly (per the test's own comment: "it doesn't give us a span unless one is active. And I didn't feel like setting that up"), and no test exercisedcreate_span()/function_trace()for any backend at all.Added
test_create_span, following the file's existing fan-out-across-backends pattern (test_set_custom_attribute,test_record_exception), callingfunction_trace()with all three backends configured and asserting each one's underlying span/trace method gets called with the function name. Ran the fullmonitoringtest suite locally: 90 passed. Also ranpycodestyle/isortagainst the changed files -- clean.Author concerns:
tag_root_span_with_error()andset_local_root_span_name()are also currently unimplemented forOpenTelemetryBackend(bothpass). I didn't touch those here --create_span()was the one with an existing consumer (function_trace(), used at ~80 call sites inedx-platform) producing a silent behavior gap; the other two didn't have an obvious "correct" OTel mapping worth guessing at in the same PR. Happy to follow up if there's interest.