Skip to content

Document every hand-written module, class, function and method in opentelemetry-sdk - #77

Closed
ocelotl wants to merge 63 commits into
docs-basefrom
docs_sdk
Closed

ocelotl wants to merge 63 commits into
docs-basefrom
docs_sdk

Conversation

@ocelotl

@ocelotl ocelotl commented Oct 30, 2026

Copy link
Copy Markdown
Owner

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

Base branch is docs-base, a snapshot of open-telemetry/opentelemetry-python@3462000f3, so the diff shows only this work. This pull request is against the ocelotl fork and is not proposed upstream.

Nothing in opentelemetry-sdk/src is generated, so all 61 source files were in scope.

What changed

61 files under opentelemetry-sdk/src/opentelemetry/sdk/ plus docs/conf.py, +10466/-142, in 65 commits.

The first 61 commits add the documentation, one file each, in reading order: the version module first, then each subpackage kept together, from the outside in. One file per commit means any single file can be reviewed on its own without reading around it.

The last four commits are repairs that the verification tools and a review pass found rather than the coverage measurement:

  • 93c54f2d0 keeps the nitpicky documentation build at its existing warning count.
  • a54707dad corrects eight docstring claims that the code contradicts, each checked against the line of code that disagrees with it. They are listed at the end of this description.
  • e26068947 restores five typing.Protocol method bodies that pyright requires, explained below.
  • 5a1bfa821 documents eleven callables that an earlier revision of the coverage script could not see, because it did not descend into functions nested inside if, try and with blocks.

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.

New text names things as inline literals in double backticks rather than as Sphinx cross-reference roles. docs/conf.py:102 sets nitpicky = True, most of what these docstrings need to name is private, module-level data or a type alias, and none of those have a target to resolve to. Cross-reference roles that were already in the files are left as they were. Three new single-backtick names in opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py, at lines 177, 178 and 326, are the exception: they name Resource, Resource.create and ResourceDetector, the module docstring above them already links all three, and the documentation build confirms they resolve.

Verification

  • Docstring coverage over opentelemetry-sdk/src, measured by walking the AST: 0 remaining gaps, down from 773 across 61 files.
  • ruff check and ruff format --check with ruff 0.16.1, the version pinned in dev-requirements.txt:23: pass on all 64 files under opentelemetry-sdk/src.
  • pytest opentelemetry-sdk/tests: 973 passed, 1 skipped, 133 subtests passed.
  • tox -e typecheck: 2 errors, both in the Python 3.14 standard library file string/templatelib.py, outside this repository. The base commit reports the same two.
  • tox -e lint-opentelemetry-sdk: 10.00/10, no messages.
  • 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.

Things to check

Four decisions reach beyond adding a docstring.

docs/conf.py is the one file outside opentelemetry-sdk/src that changed. sphinx_autodoc_typehints emits a type cross-reference for a parameter only once that parameter has a :param: entry, which napoleon generates from the Args: section, so documenting the parameters turned seven private or incubating annotations into unresolvable references. They are now in nitpick_ignore, next to the two private instrument classes already listed there for exactly the same reason. The seven are opentelemetry.sdk.trace._TracerConfig, opentelemetry.sdk.metrics._internal._MeterConfig, opentelemetry.sdk.metrics._internal._ProxyMeterConfig, opentelemetry.sdk.metrics._internal.measurement_consumer.MeasurementConsumer, opentelemetry.sdk._logs._internal._LoggerConfig, opentelemetry.sdk._logs._internal._logger_metrics.LoggerMetricsT and opentelemetry.semconv._incubating.attributes.otel_attributes.OtelComponentTypeValues. The alternative is to leave those parameters undocumented.

Four modules gain # pylint: disable=too-many-lines, because the docstrings take them past the max-module-lines=1000 set at .pylintrc:268: opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py:4, opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py:4, opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py:4 and opentelemetry-sdk/src/opentelemetry/sdk/_configuration/__init__.py:5. Three other SDK modules already carry the same line, so this follows the repository's own answer rather than inventing one. The alternatives are raising the limit repository-wide or splitting the modules.

Forty bodies whose only statement was pass or ... lose that statement, because a docstring is already a body and both ruff rule PIE790 and pylint W0107/W2301 reject the leftover once a docstring is there. Behaviour is identical in every case, and no pass that was part of a larger body was touched.

Five more were removed and then restored by e26068947. They are methods of a typing.Protocol, where pyright requires the body: with the docstring as the whole body, the method is declared to return a value and returns on no code path, which tox -e typecheck reports as reportReturnType. PIE790 does not fire inside a Protocol at all, so ruff never asked for those five to go. Each now carries ... # pylint: disable=unnecessary-ellipsis, the form the repository already uses for this exact situation at opentelemetry-sdk/src/opentelemetry/sdk/trace/_sampling_experimental/_composable.py:45 and :49. The five are in _configuration/__init__.py, trace/_sampling_experimental/_rule_based.py and trace/_tracer_metrics.py.

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exponential_histogram/__init__.py was a zero-byte file. It gains the Apache license header that every other file in the package carries, alongside its new module docstring.

Documentation that was wrong and is now fixed

  • SpanLimits wrote six of its Default: lines as {_DEFAULT_...} inside a plain string, so the braces reached the rendered page verbatim, and three of the names in them do not exist in the module at all.
  • get_aggregated_resources said initial_resource "has highest priority". It is merged first, so every detector overrides it, and Resource.create relies on that to let a detector override the default telemetry.sdk.* attributes.
  • MetricExporter.export documented a parameter named metrics, which the signature does not have.
  • The four should_sample docstrings in exemplar_filter.py documented a parameter named timestamp. The signature calls it time_unix_nano.
  • Mapping.get_lower_boundary pointed its two Raises: entries at opentelemetry.sdk.metrics.MappingUnderflowError and MappingOverflowError. Neither name exists; both classes live in the mapping.errors module and are not re-exported.
  • Measurement documented its fields under an Attributes: heading whose continuation lines were not indented, so napoleon attached them to nothing.
  • AggregationTemporality said "Can be one of the following values:" and then listed none.
  • ReadWriteLogRecord.dropped_attributes opened its Returns: section with a line containing a colon, which napoleon reads as a type annotation, so "How many attributes were discarded" was emitted as a class reference.

Docstring claims that a review pass found the code contradicting

Each was fixed in a54707dad after reading the line of code that disagrees with it.

  • BatchProcessor hands the exporter the oldest item first, not the newest. emit appends on the left and the export loop pops from the right.
  • A BatchProcessor schedule delay of 0 removes the timeout rather than the wait. The worker blocks on Event.wait(None) until an emit wakes it.
  • BoundedDict moves a rewritten key to the end, so eviction drops the key written longest ago rather than the key set first.
  • GlobalErrorHandler selects a handler when the handler class is a subclass of the class of the raised exception, the reverse of the relation the docstring stated.
  • _ViewInstrumentMatch starts a new aggregation at the current time unless the stream resets on delta collection and has been collected before. The default preferences are cumulative, so no stream resets.
  • PeriodicExportingMetricReader.shutdown collects nothing of its own, and there is no exporting thread at all when the export interval is infinite.
  • PeriodicExportingMetricReader.shutdown passes the remaining budget under the keyword timeout while MetricExporter.shutdown declares timeout_millis, so the value is swallowed by **kwargs.
  • _BaseConfigurator.configure holds no guard, so calling it twice runs _configure twice.
  • _import_opamp catches StopIteration and AttributeError only, so an ImportError from loading the entry point propagates.

@ocelotl
ocelotl force-pushed the docs_sdk branch 2 times, most recently from 594efcb to a3058bc Compare October 31, 2026 14:27
@ocelotl

ocelotl commented Oct 31, 2026

Copy link
Copy Markdown
Owner Author

Superseded by the upstream pull request open-telemetry#5869

@ocelotl ocelotl closed this Oct 31, 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