Merged
Conversation
This commit breaks tests, but it bundles the large deletions into a single commit. This makes it easier to understand the later commits that made this change.
All except opentelemetry-api, which is still vendored. The actual code has already been deleted - this commit has the changes that effected that removal, for easier understanding. The setuptools package cannot be vendored, which is why it is our only formal dependency. We needed to tweak the new requirements-prod just method to not try to add it, by passing `--no-allow-unsafe`. We cannot get rid of a runtime setup tools dependency just yet, but we maybe can in future by upgrading otel libraries. This meant we can also remove the pkg_resources hack we needed for otel deps to work when vendored.
This ensures we can have telemetry during our tests
bloodearnest
force-pushed
the
optional-otel
branch
from
cd214af to
a33dbe6
Compare
This tries to import the optional dependencies, and ensures the code handles when they are not available. It adds an explict TRACING_AVAILABLE flag, and if it's False, does no-ops in our manual tracing functions. It turns out we cannot mix and match otel libraries imported from our vendored lib and globally in pip. This is an unfortunante implementation detail of the otel implementation, the module path affects some internal identity checks on objects. So, we have our tracing module provide an abstraction. It either imports the whole otel stack from the global namespace, if found (like in dev and when installed with opensafely[tracing], but falls back to importing the core `trace` module from `opensafely._vendor.opentelemetry` if they are not found. The rest of our code imports the `trace` module from jobrunner.tracing, so it is always consistent in which version it uses. This actually resulted in a much clearer api usage.
This modifies the package metadata with an extra called "tracing", which installs the right packages. It adds a functional test that installs into a virtualenv using this extra, and that if you run a job with tracing configured, tracing does work. Also, I expanded the existing installation test, that now effectively installs without the dependencies, with a similar functional test to ensure that jobs still run in a normal installation. This caught a bug, which is fixed by adding another no-op when TRACING_AVAILABLE is False.
bloodearnest
force-pushed
the
optional-otel
branch
from
a33dbe6 to
72c1684
Compare
bloodearnest
force-pushed
the
optional-otel
branch
from
ebf7ce0 to
20603a3
Compare
bloodearnest
force-pushed
the
optional-otel
branch
from
20603a3 to
54f1983
Compare
It seems docker is install in windows CI images, but it doesn't work. Thanks, Github.
bloodearnest
force-pushed
the
optional-otel
branch
from
54f1983 to
3848494
Compare
Previously, to support vendoring otel packages, we had to fork vendoring and add support for including package metadata, so that the otel plugin system would find the right packages. With those dependencies being unvendored, we do not need that any more, so can revert to upstream. This removes all the package metadata files from our _vendor dir.
bloodearnest
force-pushed
the
optional-otel
branch
from
e806385 to
0a44424
Compare
evansd
approved these changes
Sep 25, 2025
Contributor
evansd
left a comment
evansd
left a comment
There was a problem hiding this comment.
So nice seeing all this cruft disappearing. The new trace() abstraction looks like a nice simplification as well.
StevenMaude
approved these changes
Sep 25, 2025
Contributor
StevenMaude
left a comment
StevenMaude
left a comment
There was a problem hiding this comment.
This is a really good and well-explained simplification.
I didn't really know much about how OpenTelemetry is installed for Python. So it was useful to see the explanation around how the opentelemetry-api package can be still used without the other related opentelemetry dependencies installed. Leaving opentelemetry-api vendored then means we don't need to manage the availability of the other dependencies throughout our code, but only in a few places.
Related to that, a tiny nit in the PR message:
Make most of the otel dependencies optional, except for opensafely-api, which is still vendored.
I think this should be opentelemetry-api.
Member
Author
Hah, if I had a nickel for every time I have mistyped open{telemetry/safely} in this PR... |
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
3 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.
Make most of the otel dependencies optional, except for opentelemetry-api, which is still vendored. This removes them as required dependencies, but preserves the ability to use otel with
opensafely runif needed. See #361 for more info.Significant wins:
Fixes: #361