Split tracing into vendor-neutral observability + OTel adapter#861
Split tracing into vendor-neutral observability + OTel adapter#861KirillKurdyukov wants to merge 2 commits into
Conversation
Move all non-OTel tracing plumbing (Span/TracingProvider protocols, Noop implementation, SpanName, registry, YDB attribute helpers) into a new ydb.observability package. The OTel bridge in ydb.opentelemetry becomes a concrete provider (OtelTracingProvider) that plugs into the same interface. Users can now enable tracing without any OpenTelemetry dependency by supplying a custom TracingProvider to ydb.observability.enable_tracing. A second enable call always replaces the previously installed provider. ydb.opentelemetry.tracing is kept as a thin re-export shim for backward compatibility. All SDK internal imports (retries, connection, pool, session, transaction — sync + aio) now go through ydb.observability.tracing. Added tests/tracing/test_observability_enable.py covering: default Noop state, custom provider wiring, reset semantics on double enable, disable reverting to Noop, OTel enable replacing a custom provider, and duck-typed providers satisfying the protocol. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #861 +/- ##
==========================================
+ Coverage 81.38% 81.55% +0.17%
==========================================
Files 94 96 +2
Lines 12197 12209 +12
Branches 1204 1201 -3
==========================================
+ Hits 9926 9957 +31
+ Misses 1807 1799 -8
+ Partials 464 453 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- TestSplitEndpoint — parametric test covering every branch of _split_endpoint (grpcs://, grpc://, bare host:port, valid/malformed IPv6, missing colon, non-numeric port, None). - TestBuildYdbAttrsPeerOptionals — peer tuples with only address / only port / only location / empty location, hitting the three if x is not None branches in _build_ydb_attrs. - TestSetPeerAttributes — direct unit test for the set_peer_attributes helper (previously only reached through session flows that never had a non-None peer). - TestSpanFinishCallback — success and exception paths of the streaming _finish callback. - TestOpentelemetryTracingShimReexports — proves the ydb.opentelemetry.tracing back-compat shim re-exports the exact same objects as ydb.observability.tracing. - TestOtelEntrypointHandlesMissingPackage — uses sys.modules["ydb.opentelemetry.plugin"] = None to force the ImportError branches in ydb.opentelemetry.__init__.enable_tracing / disable_tracing. - TestOtelTracingSpanBridge — direct tests for TracingSpan.set_attribute and the defensive "exit before enter" branch in _AttachContext. - TestNoopContextManager — smoke tests for the Noop context manager and NoopTracingProvider returning the shared span + empty metadata.
There was a problem hiding this comment.
Pull request overview
This PR decouples SDK tracing from OpenTelemetry by introducing a vendor-neutral ydb.observability layer (interfaces + registry + helpers) and turning ydb.opentelemetry into a concrete adapter that installs an OtelTracingProvider via the new interface. This allows tracing to work without opentelemetry installed unless the user explicitly opts into the OTel adapter.
Changes:
- Add
ydb.observabilitywithSpan/TracingProviderprotocols, a Noop provider, a provider registry, and shared SDK tracing helpers (create_ydb_span, metadata injection, etc.). - Refactor SDK core (sync + async) to import tracing helpers from
ydb.observabilityinstead ofydb.opentelemetry. - Rework
ydb.opentelemetryinto an adapter (OtelTracingProvider) and keepydb.opentelemetry.tracingas a backward-compatible re-export shim; add new tests and update docs/changelog.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ydb/retries.py | Switch internal retry spans to ydb.observability.tracing. |
| ydb/query/transaction.py | Switch query transaction tracing helpers to ydb.observability.tracing. |
| ydb/query/session.py | Switch query session tracing helpers to ydb.observability.tracing. |
| ydb/pool.py | Switch pool tracing helpers to ydb.observability.tracing. |
| ydb/connection.py | Switch trace metadata injection source to ydb.observability.tracing. |
| ydb/aio/query/transaction.py | Maintain sync/async parity by switching async transaction tracing imports. |
| ydb/aio/query/session.py | Maintain sync/async parity by switching async session tracing imports. |
| ydb/aio/pool.py | Maintain sync/async parity by switching async pool tracing imports. |
| ydb/aio/connection.py | Maintain sync/async parity by switching async metadata injection imports. |
| ydb/observability/tracing.py | New vendor-neutral tracing protocols, Noop impl, registry, and shared SDK helpers. |
| ydb/observability/init.py | New public observability entrypoints (enable_tracing, disable_tracing, exports). |
| ydb/opentelemetry/plugin.py | Implement OtelTracingProvider adapter and route enable/disable through observability. |
| ydb/opentelemetry/init.py | Keep OTel entrypoints with lazy import and improved “replace provider” semantics. |
| ydb/opentelemetry/tracing.py | Preserve backward-compatible import paths via re-export shim to observability. |
| tests/tracing/test_tracing_sync.py | Update tracing tests to use observability registry and helpers. |
| tests/tracing/test_tracing_async.py | Update async tracing tests to import from observability. |
| tests/tracing/test_observability_enable.py | Add new unit tests for provider swapping/Noop behavior without OTel. |
| docs/opentelemetry.rst | Document provider replacement semantics and the new vendor-neutral API. |
| CHANGELOG.md | Note new ydb.observability entrypoint and removal of core OTel import dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from ydb.observability import enable_tracing | ||
| from ydb.opentelemetry import OtelTracingProvider | ||
|
|
||
| enable_tracing(OtelTracingProvider()) # or any custom TracingProvider |
| def enable_tracing(provider: TracingProvider) -> None: | ||
| """Install *provider* as the active tracing backend. | ||
|
|
||
| Calling this a second time replaces the previous provider — the old one | ||
| stops receiving spans immediately. Passing ``None`` is equivalent to | ||
| :func:`disable_tracing`. | ||
| """ | ||
| _registry.set_provider(provider) |
| def get_trace_metadata(self) -> Iterable[Tuple[str, str]]: | ||
| headers: dict = {} | ||
| inject(headers) | ||
| return tuple(headers.items()) |
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Minor | High: Incorrect import path in docstring —
ydb/observability/__init__.py:9 - Minor | Medium:
enable_tracingtype annotation rejectsNonebut implementation and docs accept it —ydb/observability/__init__.py:28 - Minor | Medium: Backward-compat shim re-exports
_registrybut its type changed fromOtelTracingRegistryto_TracingRegistrywith different methods —ydb/opentelemetry/tracing.py:17 - Nit | Low:
Span.attach_contextreturn type isAnyinstead ofContextManager—ydb/observability/tracing.py:39
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
|
|
||
| from ydb.observability import enable_tracing | ||
| from ydb.opentelemetry import OtelTracingProvider | ||
|
|
There was a problem hiding this comment.
Severity: Minor
Confidence: High
The docstring suggests from ydb.opentelemetry import OtelTracingProvider, but OtelTracingProvider is only defined in ydb.opentelemetry.plugin and is not re-exported from ydb.opentelemetry.__init__. This import will raise ImportError at runtime.
Suggested fix — either:
- Change the docstring to
from ydb.opentelemetry.plugin import OtelTracingProvider, or - Add
OtelTracingProvidertoydb/opentelemetry/__init__.py's namespace (and__all__).
| ) | ||
|
|
||
|
|
||
| def enable_tracing(provider: TracingProvider) -> None: |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
The type annotation declares provider: TracingProvider, but the docstring on line 33 says "Passing None is equivalent to disable_tracing()", and _registry.set_provider accepts Optional[TracingProvider]. The test at test_observability_enable.py:193 calls enable_tracing(None) with a # type: ignore comment.
Consider changing the signature to provider: Optional[TracingProvider] so the documented behavior is expressible without type: ignore, or remove the "Passing None" sentence from the docstring if the intent is to keep None as an undocumented escape hatch.
| TracingProvider, | ||
| _NoopCtx, | ||
| _build_ydb_attrs, | ||
| _registry, |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
The backward-compat shim re-exports _registry, but its type has changed from OtelTracingRegistry to _TracingRegistry. The old registry exposed set_metadata_hook() and set_create_span(); the new one exposes set_provider() and get_provider() instead. Code that directly called _registry.set_create_span(...) or _registry.set_metadata_hook(...) (or imported OtelTracingRegistry) will break.
All affected names are private (_-prefixed), so this is acceptable, but it may be worth noting in the changelog or migration notes that direct use of the internal registry API is no longer supported.
|
|
||
| def end(self) -> None: ... | ||
|
|
||
| def attach_context(self, end_on_exit: bool = True) -> Any: |
There was a problem hiding this comment.
Severity: Nit
Confidence: Low
attach_context returns Any. A more precise return type (e.g. ContextManager["Span"] from typing) would give static-analysis tools and IDE users better guidance about the returned context manager.
|
Analysis performed by claude, claude-opus-4-6. |
Summary
Fixes an architectural issue: tracing was tightly coupled to OpenTelemetry — the SDK forced users to have
opentelemetryinstalled just to enable tracing, and there was no way to plug in a custom backend.This PR splits the module in two:
ydb.observability(new) — vendor-neutral interfaces (Span,TracingProvider), Noop implementation,SpanName, registry, and all SDK-side helpers (create_ydb_span,set_peer_attributes,span_finish_callback,get_trace_metadata). The SDK core imports only from here and never touchesopentelemetry.ydb.opentelemetry— becomes a concreteOtelTracingProviderthat plugs into the same interface.ydb.opentelemetry.enable_tracing(tracer=None)is now a thin convenience that builds the provider and hands it toydb.observability.enable_tracing(...).New public API
enable_tracingcleanly resets the previously installed provider (works for OTel and custom providers alike — previouslyydb.opentelemetry.enable_tracingwas silently idempotent).ydb.opentelemetry.tracingremains as a re-export shim for backward compatibility.Tests
tests/tracing/test_observability_enable.py(12 tests) uses a hand-rolledRecordingProvider— no OTel involvement — to cover:enable_tracing,enable_tracing(None)==disable_tracing(),disable_tracingreverts to Noop,ydb.opentelemetry.enable_tracingreplacing a custom provider,TracingProviderprotocol.Test plan
tox -e black-format+tox -e blacktox -e styletox -e mypytox -e py -- ydb -v(174 passed)tox -e py -- tests/tracing -v(55 passed, including 12 new observability tests)🤖 Generated with Claude Code