Skip to content

[3008.x] Defer OpenTelemetry imports in salt.utils.tracing / salt.utils.metrics (#69855)#69856

Merged
dwoz merged 1 commit into
saltstack:3008.xfrom
dwoz:dwoz/fix/issue-69855-3008x
Jul 23, 2026
Merged

[3008.x] Defer OpenTelemetry imports in salt.utils.tracing / salt.utils.metrics (#69855)#69856
dwoz merged 1 commit into
saltstack:3008.xfrom
dwoz:dwoz/fix/issue-69855-3008x

Conversation

@dwoz

@dwoz dwoz commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #69855.

salt.utils.tracing and salt.utils.metrics unconditionally imported the entire OpenTelemetry SDK at module load, even though tracing.enabled and metrics.enabled both default to false. Because both modules are imported transitively by every daemon entry point (salt.master, salt.minion, salt.channel.client, salt.channel.server, salt.utils.event, salt.netapi.rest_cherrypy.app, salt.utils.reactor), a ~15-process salt-master container was paying ~15 MB per subsystem per process — roughly 450 MB total — for functionality nobody was using.

This landed on 3008.x and shows up on the nightly stress rig as a +300 MB baseline / +430 MB peak container RSS shift versus 3006.x under otherwise-identical workload:

3006.x 3008.x Δ
Master container RSS, mean 1352 MB 1649 MB +297 MB
...median 1366 MB 1712 MB +346 MB
...p90 1529 MB 1938 MB +409 MB
...p99 1650 MB 2065 MB +415 MB
...peak 1683 MB 2112 MB +429 MB

(3008.x nightly: https://github.com/saltstack/salt/actions/runs/29970269739. 3006.x nightly: https://github.com/saltstack/salt/actions/runs/28280683908.)

Each ~300 MB salt engine child inherits the same overhead (engines run as separate Python processes that re-import salt.master / salt.minion).

Change

Move the OTel imports into _load_otel() helpers invoked only after is_enabled() returns True. is_enabled() short-circuits on _cached_opts being None or enabled being false, so the probe (and therefore the OTel import) doesn't fire on the default disabled path. Public API is preserved; SpanKind remains a stub always and is translated to the real trace.SpanKind enum inside start_span() when tracing is on.

Per-process measurement of the imports removed from module top (Python 3.10 slim):

before:                        14804 KB
after tracing.py imports:      30036 KB

So each salt process trims by ~15 MB (tracing) + ~15 MB (metrics) = ~30 MB. Across the master container's ~15 Python processes that's ~450 MB reclaimed on a stack that has tracing/metrics disabled.

What issues does this PR fix or reference?

Fixes: #69855

Merge requirements satisfied?

  • Changelog: changelog/69855.fixed.md
  • Tests written / updated:
    • tests/pytests/unit/utils/test_tracing.py — new subprocess tests:
      • test_import_does_not_load_opentelemetry — importing salt.utils.tracing (and calling the disabled-path public API) must not put any opentelemetry.* entries in sys.modules.
      • test_enabling_tracing_loads_opentelemetry_lazily — the mirror: configure({..., enabled: True}) does trigger the import and a real span comes back.
      • test_master_and_minion_imports_do_not_load_opentelemetry — end-to-end guard: importing the whole daemon import chain (salt.master, salt.minion, salt.channel.*, salt.utils.event, salt.utils.tracing) leaves opentelemetry unimported. Catches any future eager top-level import elsewhere in the chain.
    • tests/pytests/unit/utils/test_metrics.py — matching pair: test_import_does_not_load_opentelemetry and test_enabling_metrics_loads_opentelemetry_lazily.
    • Existing test_module_works_when_opentelemetry_missing tests updated to reflect the new tri-state _OTEL_AVAILABLE (None until first probe, then True/False).
  • pre-commit clean on all changed files.

Both modules unconditionally imported the OTel SDK at module load,
even though ``tracing.enabled`` and ``metrics.enabled`` default to
false. Every salt daemon entry point transitively imports both
modules (via salt.master, salt.minion, salt.channel.*,
salt.utils.event, salt.netapi.rest_cherrypy.app), so a ~15-process
salt-master container was paying ~15 MB per subsystem per process --
~450 MB total -- for functionality nobody was using. This showed up
as a +300 MB baseline / +430 MB peak container RSS shift versus
3006.x on the nightly stress rig, and each ~300 MB salt engine
child inherited the same overhead.

Move the OTel imports into ``_load_otel()`` helpers invoked only
after ``is_enabled()`` returns True, gated on ``_cached_opts`` (which
is populated by ``configure()`` but requires the "enabled" key to be
true for probes to fire). Public API is preserved; ``SpanKind``
remains a stub always and is translated to the real enum inside
``start_span()``.
dwoz added a commit that referenced this pull request Jul 23, 2026
Both modules unconditionally imported the OTel SDK at module load,
even though tracing.enabled and metrics.enabled default to false.
Every salt daemon entry point transitively imports both modules, so
a ~15-process salt-master container was paying ~15 MB per subsystem
per process (~450 MB total) for functionality nobody was using.

Move the OTel imports into _load_otel() helpers invoked only after
is_enabled() returns True. Public API is preserved.

Backport of 73bca44 from PR #69856.
@dwoz
dwoz merged commit cc2b757 into saltstack:3008.x Jul 23, 2026
954 of 963 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants