Repository navigation
Conversation
Pull request dashboard statusWaiting on reviewers · refreshed 2026-11-01 16:19 UTC Review the latest changes. Status above doesn't look right?
|
This branch has not been deployed
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
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.
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 aReturns:section.No behaviour changes. Nothing under
opentelemetry-api/srcis 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:
versionandenvironment_variablesfirst, then the sixutilmodules, the threecontextmodules,attributes, the twobaggagemodules, the threepropagatorsmodules,propagate, thentracewithspan.py,status.pyand the two trace propagation modules, then the two_logsmodules, and the threemetricsmodules 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.pydoes not setspecial-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.
opentelemetry-api/src, measured by walking the AST and ignoring@overloadstubs: 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 whoseArgs:section omits a parameter, and for a callable with a return value and noReturns:section. The 304 break down as 22 modules, 22 classes and 158 functions with no docstring, 45 incomplete parameter lists and 57 missingReturns:sections.ruff checkandruff format --checkwith ruff 0.16.1, the version pinned atdev-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 runssphinx-build -Wwithnitpicky = Trueset atdocs/conf.py:102: 16 warnings, the same 16 as the base commit, of the same four kinds (6_http.Compression, 6 ambiguousMeterProvider, 2types.Attributes, 2Attributes). This branch adds none. The command exits 1 on the base commit too, because-Wturns those 16 into errors.tox -e typecheck: 2 errors, both in the CPython 3.14 standard library filestring/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:268setsmax-module-lines=1000, sotox -e lint-opentelemetry-apifails without both. The new docstrings takeopentelemetry-api/src/opentelemetry/metrics/_internal/__init__.pyfrom 892 to 1293 lines andopentelemetry-api/src/opentelemetry/metrics/_internal/instrument.pyfrom 540 to 1017 lines. The alternatives are raisingmax-module-linesrepository-wide or splitting the two modules.format_trace_idandformat_span_idinopentelemetry-api/src/opentelemetry/trace/span.pyputArgs: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: intand:param trace_id: Trace ID intwhere 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_spansaid every re-raised exception is recorded, when it records only for a recording span and only whenrecord_exceptionis true._ProxyMeter,_ProxyMeterProvider,ProxyTracerProviderandProxyLoggerProvidereach said every object they hand out is a proxy, when all four return the real object once one is installed.Statussaid a description accompanying a non-error code is always discarded with a warning, when an empty one skips every check._is_valid_pairsaid 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.Oncesaid 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.
LogRecordsaidobserved_timestampfalls back totimestamp; it does not.Logger.emitsaidtimestampdefaults to the creation time; it staysNone.NoOpLogger.emitinherited 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.TraceStateiteration was described as most-recently-added first; it follows the supplied order. Thecontextvarsmodule 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. Theenvironment_variablesmodule said it holds the names the API reads, while four of its nine names are read only byopentelemetry.sdk._configuration. Thecompositemodule said aCompositePropagatoris returned whenOTEL_PROPAGATORSnames more than one propagator; it is returned in every case, including when the variable names one and when it namesnone.A base implementation that only warns, documented as though it worked.
Meter.create_gaugepromised aGaugereturn while its own body emits a warning and returnsNone.Span.add_linksaid "Adds aLink" while its own body emits a warning and adds nothing.An argument the callee accepts and discards.
NoOpTracer.start_spanandstart_as_current_span,NoOpLogger.enabled, the four no-op instrument methods, and theattributesparameter ofNoOpMeterProvider.get_meterand_ProxyMeterProvider.get_meterall 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.detachraisesValueErrorfor a token from a different instance andRuntimeErrorfor 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_instrumentstill described a tuple return that the function stopped having._check_name_unit_descriptiondocumented two parameters as acceptingNonewhile their annotations arestr, which is now stated as the defensive handling it is.functools.cachewas 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
Statusdescription rule, in the module docstring oftrace/status.py; the baggage size limits, in the module docstring ofbaggage/propagation/__init__.py, where two of the three limits also discard every entry after the offending one; and theOnceexactly-once claim, twice, in the module docstring and then in the class docstring.reStructuredText that did not render as intended. A
TraceFlagsflag list that rendered as one paragraph, adeprecateddirective 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 aLogRecord.__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_EXPORTERdocstring declared the reStructuredText link targetGeneral SDK Configuration, whichOTEL_METRICS_EXPORTERalready declared with a different URL. That is a[docutils]duplicate-target warning, so it took the build to 17 warnings and failedsphinx-build -W. It now uses the anonymous__link form, which declares no named target.Known limitation
default_role = "any"is set atdocs/conf.py:64and is destroyed bysphinx_autodoc_typehintsbefore 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-799runsRstParser().parse(...)inside theautodoc-process-docstringhandler, anddocutils/parsers/rst/__init__.py:186-187deletesroles._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 workinganycross-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.