Skip to content

Document all hand-written source in opentelemetry-api - #54

Closed
ocelotl wants to merge 29 commits into
docs-basefrom
docs_api_docstrings
Closed

ocelotl wants to merge 29 commits into
docs-basefrom
docs_api_docstrings

Conversation

@ocelotl

@ocelotl ocelotl commented Oct 30, 2026 •

Copy link
Copy Markdown
Owner

Documents every hand-written module, class, function and method in opentelemetry-api, so that each one carries a docstring, every parameter is documented, and every callable with a return value has a Returns: section.

No behaviour changes. Nothing under opentelemetry-api/src is generated, so all 28 source files were in scope and all 28 changed.

What changed

28 files under opentelemetry-api/src/opentelemetry/, +2864/-81, in 28 commits, one per file, plus a final commit adding the changelog fragment.

How to review

There is exactly one commit per file, so any single file can be reviewed on its own without reading around it. The order is dependency order: version and environment_variables first, then the six util modules, the three context modules, attributes, the two baggage modules, the three propagators modules, propagate, then trace with span.py, status.py and the two trace propagation modules, then the two _logs modules, and the three metrics modules last.

If 28 files in one pull request is too much at once, I am happy to split this into one pull request per subpackage (trace, metrics, _logs, context, baggage, propagators, util). Say the word and I will close this and open them separately. I kept it together because the conventions below only make sense judged as a whole.

Conventions followed

docs/conf.py does not set special-members, so an __init__ docstring never renders. Constructor parameters therefore go on the class docstring, which is what the API reference shows, and __init__ carries a short docstring saying so. That keeps one parameter list that cannot drift out of step with a second copy. The convention is applied to every class in the package that takes constructor arguments, 31 of them, and the rendered pages were checked afterwards to confirm that no parameter was lost and no class gained a second Parameters list.

Verification

Every command below was run at the head of this branch.

  • Docstring coverage over opentelemetry-api/src, measured by walking the AST and ignoring @overload stubs: 0 remaining gaps, down from 304 at the base commit. A gap is counted for a module, class, function or method with no docstring, for a docstring whose Args: section omits a parameter, and for a callable with a return value and no Returns: section. The 304 break down as 22 modules, 22 classes and 158 functions with no docstring, 45 incomplete parameter lists and 57 missing Returns: sections.
  • ruff check and ruff format --check with ruff 0.16.1, the version pinned at dev-requirements.txt:23: pass on all 30 files.
  • tox -e lint-opentelemetry-api: 10.00/10, no messages.
  • pytest opentelemetry-api/tests: 300 passed, 1 skipped, 127 subtests passed. Identical to the base commit.
  • tox -e docs, which runs sphinx-build -W with nitpicky = True set at docs/conf.py:102: 16 warnings, the same 16 as the base commit, of the same four kinds (6 _http.Compression, 6 ambiguous MeterProvider, 2 types.Attributes, 2 Attributes). This branch adds none. The command exits 1 on the base commit too, because -W turns those 16 into errors.
  • tox -e typecheck: 2 errors, both in the CPython 3.14 standard library file string/templatelib.py, naming no file in this repository. The base commit reports the same two, so this command is not currently usable as a gate on any branch.

Decisions that go beyond adding a docstring

These are the places where a reviewer's judgment is actually needed.

Two modules gain # pylint: disable=too-many-lines, the only two comment lines this branch adds anywhere. .pylintrc:268 sets max-module-lines=1000, so tox -e lint-opentelemetry-api fails without both. The new docstrings take opentelemetry-api/src/opentelemetry/metrics/_internal/__init__.py from 892 to 1293 lines and opentelemetry-api/src/opentelemetry/metrics/_internal/instrument.py from 540 to 1017 lines. The alternatives are raising max-module-lines repository-wide or splitting the two modules.

format_trace_id and format_span_id in opentelemetry-api/src/opentelemetry/trace/span.py put Args: on the line straight after the summary. Napoleon needs a blank line before a section header, so it never recognised the section and emitted the converted field list as literal text: the rendered trace page showed :type trace_id: int and :param trace_id: Trace ID int where a Parameters block belongs. Both predate this branch. The blank line is added, and both summaries gain the full stop that every other summary in the file carries.

Documentation that was wrong and is now corrected

Found while writing the surrounding docstrings, each checked against the line of code that disagrees with it. They group by the kind of error.

Stated as unconditional something the code does conditionally. use_span said every re-raised exception is recorded, when it records only for a recording span and only when record_exception is true. _ProxyMeter, _ProxyMeterProvider, ProxyTracerProvider and ProxyLoggerProvider each said every object they hand out is a proxy, when all four return the real object once one is installed. Status said a description accompanying a non-error code is always discarded with a warning, when an empty one skips every check. _is_valid_pair said a trace state key may start with a lowercase letter or a digit, when the grammar permits a leading digit only in the tenant @ vendor form. Once said it executes a function exactly once, when a call that raises leaves the instance unmarked so the next call runs the callable again.

Stated something the code does not do at all. LogRecord said observed_timestamp falls back to timestamp; it does not. Logger.emit said timestamp defaults to the creation time; it stays None. NoOpLogger.emit inherited the wording that a record is built from the keyword arguments; its body is empty, so no record is built, no timestamp is taken and no context is consulted. TraceState iteration was described as most-recently-added first; it follows the supplied order. The contextvars module said a generator gets its own context view; PEP 567 gives one to a thread and to a task, not to a generator, and a value a generator attaches is visible to its caller. The environment_variables module said it holds the names the API reads, while four of its nine names are read only by opentelemetry.sdk._configuration. The composite module said a CompositePropagator is returned when OTEL_PROPAGATORS names more than one propagator; it is returned in every case, including when the variable names one and when it names none.

A base implementation that only warns, documented as though it worked. Meter.create_gauge promised a Gauge return while its own body emits a warning and returns None. Span.add_link said "Adds a Link" while its own body emits a warning and adds nothing.

An argument the callee accepts and discards. NoOpTracer.start_span and start_as_current_span, NoOpLogger.enabled, the four no-op instrument methods, and the attributes parameter of NoOpMeterProvider.get_meter and _ProxyMeterProvider.get_meter all documented arguments that reach no telemetry. Each is now marked as ignored, with what the body actually reads.

An exception the callable propagates, undocumented. The three global provider getters re-raise whatever loading the entry point named by the environment variable raised, and a name matching no installed entry point surfaces as StopIteration. ContextVarsRuntimeContext.detach raises ValueError for a token from a different instance and RuntimeError for a token already used.

Something the code passes on, omitted from a list that reads as exhaustive. Four of the seven proxy instruments said the real instrument carries "the name, unit and description this proxy was built with", while three of them also pass the callbacks and the fourth also passes the bucket advisory. A reader took the opposite of the truth in the one case the proxy exists for: that an observable instrument created before the SDK is installed has lost its callbacks, and that a histogram created the same way has lost its boundaries.

Pre-existing documentation that disagreed with the code. _register_instrument still described a tuple return that the function stopped having. _check_name_unit_description documented two parameters as accepting None while their annotations are str, which is now stated as the defensive handling it is. functools.cache was described as if it serialised the first read, when it holds no lock across the wrapped call.

A claim corrected in one docstring and left standing in another docstring of the same file. Found four times, each time by sweeping one level of the documentation against another: the Status description rule, in the module docstring of trace/status.py; the baggage size limits, in the module docstring of baggage/propagation/__init__.py, where two of the three limits also discard every entry after the offending one; and the Once exactly-once claim, twice, in the module docstring and then in the class docstring.

reStructuredText that did not render as intended. A TraceFlags flag list that rendered as one paragraph, a deprecated directive with no version, an argument order and a syntax error in two docstring examples, a hyperlink target referenced nowhere so its URL never appeared, and a LogRecord.__init__ docstring placed after the first statements of the body, which made it a pointless string statement rather than a docstring.

One of these is worth calling out, because it is the kind a warning count can hide. A first draft of the OTEL_LOGS_EXPORTER docstring declared the reStructuredText link target General SDK Configuration, which OTEL_METRICS_EXPORTER already declared with a different URL. That is a [docutils] duplicate-target warning, so it took the build to 17 warnings and failed sphinx-build -W. It now uses the anonymous __ link form, which declares no named target.

Known limitation

default_role = "any" is set at docs/conf.py:64 and is destroyed by sphinx_autodoc_typehints before any autodoc'd docstring is parsed, so every single-backtick reference in a docstring renders as <cite> rather than as a link. sphinx_autodoc_typehints/__init__.py:798-799 runs RstParser().parse(...) inside the autodoc-process-docstring handler, and docutils/parsers/rst/__init__.py:186-187 deletes roles._roles[''] at the end of every parse, unregistering the default role for the rest of the document. The whole build contains 275 <cite> elements against 23 working any cross-references. 97 of the single-backtick references predate this branch and 32 are added by it, so the convention is not introduced here, and the explicit roles this branch uses elsewhere (:class:, :meth:, :func:) are unaffected and do render as links. Converting all 129 is a separate change to the whole package rather than part of this one.

@ocelotl
ocelotl force-pushed the docs_api_docstrings branch from 53f9425 to 43bd5d1 Compare November 1, 2026 16:00
@ocelotl

ocelotl commented Nov 2, 2026

Copy link
Copy Markdown
Owner Author

Superseded by open-telemetry#5876, which carries the same commits against upstream main. The branch stays, because that pull request uses it as its head.

@ocelotl ocelotl closed this Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant