Repository navigation
Conversation
Pull request dashboard statusWaiting on reviewers · refreshed 2026-10-31 18:39 UTC Review the latest changes. Status above doesn't look right?
|
herin049
left a comment
There was a problem hiding this comment.
I think documenting our public API better is a good thing, but I'm not sure that we get much value from documenting our private API.
I am already working on this same fix for the API. This is not only documenting our private API (which I actually think is still a good thing), it also fixes documentation errors and documents objects that are also public. In general, I think there is no such thing as too much documentation. |
I'm mostly just concerned with added bloat to the code base. My experience with Claude (and other models) is that they are often overly verbose when it comes to comments which sometimes make it harder to sift through the code base. Another approach we could consider for LLM generated documentation is something like DeepWiki, or other forms of decoupling our docs from our code, have you looked into these types of solutions? |
I haven't, they could be a better approach. Even so, our documentation right now is in pretty bad shape. This new documentation may be a bit bloated but it is definitely better than missing and wrong documentation. I think we should get our docs to a good, stable place first, then we can consider other options. I prefer this approach of fixing the problem now, optimizing later because deciding on another approach requires consensus and that is time consuming, which is fine but it is better to make those decisions when the current, more serious problem has been fixed already. |
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 aReturns:section.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.New text names things as inline literals in double backticks rather than as Sphinx cross-reference roles.
docs/conf.py:102setsnitpicky = 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 already in the files are left as they were. Three new single-backtick names inopentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py, at lines 177, 178 and 326, are the exception: they nameResource,Resource.createandResourceDetector, the module docstring above them already links all three, and the documentation build confirms they resolve.Decisions that go beyond adding a docstring
These five are the places where a reviewer's judgment is actually needed.
docs/conf.pyis the one file outsideopentelemetry-sdk/srcthat changed.sphinx_autodoc_typehintsemits a type cross-reference for a parameter only once that parameter has a:param:entry, which napoleon generates from theArgs:section, so documenting the parameters turned seven private or incubating annotations into unresolvable references. They are now innitpick_ignore, next to the two private instrument classes already listed there for exactly the same reason. The seven areopentelemetry.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.LoggerMetricsTandopentelemetry.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 themax-module-lines=1000set at.pylintrc:268:resources/__init__.py,metrics/_internal/__init__.py,metrics/_internal/export/__init__.pyand_configuration/__init__.py. Three other SDK modules already carry the same line, so this follows the repository's existing answer rather than inventing one. The alternatives are raising the limit repository-wide or splitting the modules.Thirty-five bodies whose only statement was
passor...lose that statement, because a docstring is already a body and both ruff rule PIE790 and pylintW0107andW2301reject the leftover once a docstring is there. Behaviour is identical in every case, and nopassthat was part of a larger body was touched.Five
typing.Protocolmethods keep their.... With the docstring as the whole body, the method is declared to return a value and returns on no code path, which pyright reports asreportReturnType. PIE790 does not fire inside aProtocol, so ruff never asked for those five to go. Each carries... # pylint: disable=unnecessary-ellipsis, the form this repository already uses for the same situation atopentelemetry-sdk/src/opentelemetry/sdk/trace/_sampling_experimental/_composable.py:45and:49. The five are in_configuration/__init__.py,trace/_sampling_experimental/_rule_based.pyandtrace/_tracer_metrics.py.opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exponential_histogram/__init__.pywas 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 corrected
Found while writing the surrounding docstrings, each checked against the line of code that disagrees with it.
SpanLimitswrote six of itsDefault:lines as{_DEFAULT_...}inside a plain string rather than an f-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_resourcessaidinitial_resource"has highest priority". It is merged first, so every detector overrides it, andResource.createrelies on that to let a detector override the defaulttelemetry.sdk.*attributes.MetricExporter.exportdocumented a parameter namedmetrics, which the signature does not have.should_sampledocstrings inexemplar_filter.pydocumented a parameter namedtimestamp. The signature calls ittime_unix_nano.Mapping.get_lower_boundarypointed its twoRaises:entries atopentelemetry.sdk.metrics.MappingUnderflowErrorandMappingOverflowError. Neither name exists; both classes live in themapping.errorsmodule and are not re-exported.Measurementdocumented its fields under anAttributes:heading whose continuation lines were not indented, so napoleon attached them to nothing.AggregationTemporalitysaid "Can be one of the following values:" and then listed none.ReadWriteLogRecord.dropped_attributesopened itsReturns: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.BatchProcessorhands the exporter the oldest item first, not the newest.emitappends on the left and the export loop pops from the right.BatchProcessorschedule delay of 0 removes the timeout rather than the wait. The worker blocks onEvent.wait(None)until anemitwakes it.BoundedDictmoves a rewritten key to the end, so eviction drops the key written longest ago rather than the key set first.GlobalErrorHandlerselects a handler when the handler class is a subclass of the class of the raised exception, the reverse of the relation the docstring stated._ViewInstrumentMatchstarts 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.shutdowncollects nothing of its own, and there is no exporting thread at all when the export interval is infinite.PeriodicExportingMetricReader.shutdownpasses the remaining budget under the keywordtimeoutwhileMetricExporter.shutdowndeclarestimeout_millis, so the value is swallowed by**kwargs._BaseConfigurator.configureholds no guard, so calling it twice runs_configuretwice._import_opampcatchesStopIterationandAttributeErroronly, so anImportErrorfrom loading the entry point propagates.Known limitation
_check_span_endedinopentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.pyapplies nofunctools.wraps, so the wrapper's name, signature and docstring replace those ofSpan.set_status,Span.update_name,Span._add_eventandSpan._add_linkin the rendered reference. Addingfunctools.wrapsis a code change, so instead the wrapper docstring now names the four methods it stands in for and says that the reference shows this text in place of theirs. Worth fixing properly in a follow-up.