Repository navigation
Conversation
….type The database, messaging, RPC and HTTP semantic conventions all mark error.type as Conditionally Required when an operation fails, on the span describing the operation and on the operation duration histogram alike, and require the two to carry the same value. Add _set_error_type, which writes the canonical class name of an exception to either destination, and _record_error_type, a context manager that applies it to a block and re-raises the original exception unmodified. Only Exception is caught, so an asyncio.CancelledError passes through without being recorded as a failure. Every instrumentation commit that follows uses one of these two. Which one it uses is the clearest signal of the shape of that instrumentation: a package that already has an exception handler calls _set_error_type from inside it, and a package that has only a bare call site wraps that call site in _record_error_type.
Unlike every other package in this branch, dbapi was already recording error.type. It recorded it in one place only, the attribute dictionary of the duration histogram, so a failed query produced a metric point carrying error.type and a span that did not. The semantic conventions require a span and its corresponding metric for one operation to carry the same value. traced_execution already captures the exception in order to pass it to _record_metrics, so the fix is one call to _set_error_type in the handler that is already there, in the sync and the async variant.
…d query The simplest shape in this branch, and the one worth reading first. _traced_execute_async sets its attributes and then calls the driver on a bare line inside the span context manager, with no exception handling of its own. Wrapping that one line in _record_error_type is the whole change. Nothing else in the package needed touching, because the driver call appears exactly once.
…ed command Same shape as cassandra with one wrinkle. _wrap_cmd already contains a try/except, and that handler is not about the memcached command at all: it guards the attribute collection and logs a warning if building the span attributes fails. The command itself runs on the line after the handler, outside it. So the call site to wrap is the return statement following the existing except clause, not the body inside it. Tagging inside the existing handler would have recorded a failure of the instrumentation as a failure of the operation.
…mmand Four call sites rather than one: a command and a pipeline wrapper, each in a sync and an async variant. The two pipeline wrappers are the interesting pair. They already catch redis.WatchError and respond by setting the span status to UNSET and re-raising after the span has closed, because a WatchError means the watched key changed and the caller should retry rather than that the pipeline failed. Wrapping the whole try/except in _record_error_type preserves that: the inner handler takes the WatchError first, so it never reaches the context manager and never gets tagged, while any other exception passes through the try unhandled and does get tagged. A test asserts that a WatchError leaves every span in the pipeline without error.type, so the distinction cannot regress silently.
…ed statement sqlalchemy needs no call site wrapped at all, because it does not wrap a call. It subscribes to the engine event hooks, and failures arrive at a dedicated _handle_error callback carrying the exception as context.original_exception. That callback already sets the span status to ERROR from that exception, so the fix is one call to _set_error_type beside the line that was already reading the exception.
…led operation Two near-identical blocks, one for the ORM operation and one for the pydantic queryset serialisation, each already capturing the exception into a local and already setting the span status to ERROR in a finally clause. _set_error_type goes beside each of those two set_status calls. This is the only package in the branch without a test. Its suite hangs in this environment on unmodified source, so a test added here could not be shown to fail before the change and pass after it, which is the check every other commit in this branch rests on.
… operation Two decorators, and they take the two different forms _record_error_type supports. The publish decorator already opens a span context manager and then injects headers before awaiting the publish, so the awaited call is wrapped on its own line inside that block. The callback decorator has nothing between opening the span and awaiting the consumer callback, so the context manager joins the existing with statement rather than adding a nesting level.
… send This package could not record error.type without a behaviour change first, which makes it the odd one out among the messaging instrumentations. _traced_send opened a producer span, enriched it, injected the headers, closed the span, and only then awaited the send. The span therefore measured the header injection and ended before the operation it names had started, so no failure of the send could ever reach it. Compare the equivalent in kafka-python, which keeps the call inside. Move the awaited call inside the span and wrap it. The span now covers the send, which is what a producer span is supposed to measure, and the duration it reports changes accordingly.
… operation Three wrappers, and the batch one is the reason this package uses both helpers. send_message and receive_message each hold a single span open around a single call, so _record_error_type wraps the call. send_message_batch does not. It starts one span per entry in the batch, collects them in ids_to_spans, makes one call for the whole batch, and ends every span afterwards. That afterwards never ran when the call raised, so an exception left every span in the batch open and unended, never exported. The explicit handler tags each span with _set_error_type and ends it before re-raising, which fixes the leak as well as the missing attribute.
celery needs no call site wrapped, because it hooks signals rather than wrapping calls. _trace_failure already receives the failure, and already does real work to find the exception worth reporting: it unwraps billiard's ExceptionInfo and ExceptionWithTraceback to reach the exception the task actually raised, and it returns early for an exception listed in the task's own throws, which Celery treats as an expected outcome. _set_error_type goes after that unwrapping and after that early return, so the recorded value is the canonical class name of the real exception and an expected throws exception stays untagged. The duration metric is untouched. The only instrument this package records is flower.task.runtime.seconds, which is specific to this instrumentation rather than an operation duration histogram defined by the messaging conventions, so the rule about error.type on a duration metric does not reach it.
… failed produce One call site, in wrap_produce, with the same shape as cassandra: attributes are set, headers are injected, and the produce call is returned on a bare line inside the span. Only the producer path is changed. The consumer path opens its span in wrap_poll through _create_new_consume_span and closes it on the following poll rather than around a call that can raise, so there is no operation boundary there to attach a failure to.
…iled send One call site in _traced_send. Note what is left outside the wrapper: the produce hook immediately above it already has its own try/except that logs and swallows, so a user hook that raises is not a failed send and is not tagged. Only the call to the kafka client is wrapped. The lines after the wrapped call read the partition off the returned future and enrich the span, and they stay outside, because they only run when the send succeeded.
…ration Two call sites, the consumer callback and basic_publish, both following the same pattern as kafka-python: a user hook with its own swallowing handler sits immediately above the real call, and only the real call is wrapped. The publish decorator has an early return above the span for the case where _get_span declines to create one, which is how this package expresses suppression. That path is untouched and still returns the original function's result directly.
…d message The missing attribute was the smaller of two problems here. after_process_message and after_enqueue both already received the failure as an exception keyword argument, and both discarded it, calling activation.__exit__(None, None, None) unconditionally. A message that raised therefore produced a span with no error status, no exception event and no error.type, which is indistinguishable from a message that succeeded. Every other package in this branch at least had the status right. Add _exit_activation, which passes the exception to the activation when there is one so that use_span sets the status and records the event, and tags the span first. after_enqueue also had to start binding the span, which it was discarding into _.
The largest change in the branch, and the only one where the recorded value is not the canonical class name of an exception. The RPC conventions ask for the gRPC status code when the call returned one, as in DEADLINE_EXCEEDED, and fall back to the exception type only when the call failed before any status code existed. _set_grpc_error_type makes that choice. It takes a status code directly when the caller has one, pulls the code off a grpc.RpcError when the caller has an exception, and delegates to _set_error_type otherwise. Twelve error paths across the sync and async client and server interceptors call it. _set_server_status exists for a second reason specific to the server side. The three servicer context methods that set a span status route through _server_status, which maps only six of the gRPC status codes to an error and returns UNSET for the rest. So error.type cannot simply be written whenever a code is present; it has to follow the same decision, and now does, in one place rather than six. The uncaught-exception handlers keep their existing guard on type(error) != Exception. A bare Exception there is a gRPC abort that the servicer context wrapper has already reported, and tagging it again would overwrite the status code with the word Exception.
…request This package already carried a TODO asking for exactly this change, so the gap was known rather than overlooked. The request ran on a bare line inside the span context manager with no handler of any kind around it, which meant a connect timeout or a connection reset produced a span with a status of Error and no error.type at all. The attribute is gated on _report_new, the way every other HTTP package in this repository gates it, because error.type belongs to the stable HTTP conventions and must not appear when only the old experimental conventions are requested. The duration metric is still not recorded when the request fails, because the call that builds its attributes needs the response object. That is a separate change.
…tion metric The span was already correct here, so this is the only commit in the branch that changes a metric and leaves the span alone. The attribute was reaching the span and never the http.server.request.duration histogram, which left the two signals disagreeing about whether the same request failed. The cause was ordering. duration_attrs is built from attributes before the handler runs, so the write in the exception handler landed in a dict that nothing read again, and add_response_attributes, which fills in error.type from the response status code, ran after both histograms had already been recorded. Moving the two recording blocks to the end of the finally lets both sources reach the metric, and the exception class name overwrites the status code on the metric exactly as it already did on the span. The write to attributes in the handler is removed rather than redirected. Nothing read attributes after duration_attrs was built from it, so that line had no effect at all.
… the batch path Two gaps in the batch path, both found by reviewing the commit that introduced the error.type handling there. The spans ended by the exception handler carried error.type but kept a status of Unset and no exception event, so a backend counting failures by span status read the batch as a success while the attribute said otherwise. The sibling single-message path gets both for free from start_as_current_span, which is why the difference was easy to miss. SQS also reports a per-entry publish failure in the Failed list of a call that returns normally. Those entries never reached any error handling, so an entry rejected with InvalidParameterValue produced a span indistinguishable from a successful publish. The Code field is the low cardinality identifier the conventions ask for, so it becomes the error.type value. The existing test_send_message_batch_all_failed asserted the exact attribute set of such a span, which pinned the absence of the attribute; it now pins its presence.
…hrough untagged The helper added by the earlier remoulade commit tagged any exception it was handed, and remoulade hands it more than failures. Its worker ends process_message with except BaseException, and an interrupted message arrives as Interrupt or one of its subclasses Shutdown and TimeLimitExceeded, all of which derive from BaseException rather than Exception. Stopping a worker while an actor ran therefore produced a consumer span attributing the stop to the actor as error.type Shutdown. This is the rule _record_error_type already documents and pins with a test for the other packages in this branch. remoulade could not use that helper, because it has no call site to wrap, so the rule has to be restated here.
Three call sites changed by this branch had no test, so deleting the change left the suite green. Each new test was checked that way: revert the one line, watch it fail with a missing error.type, restore it. aio-pika and pika each gained the attribute on the consumer callback as well as on the publish path, and only the publish path was covered. The two existing aio-pika callback tests mock _get_span away, so they can never observe an attribute; the new class starts a real span instead, mirroring TestPublishErrorType in the sibling module. pyramid gets two tests rather than one. Moving the metric recording to the end of the finally means the duration metric now carries error.type for a response that failed with no exception raised, which the branch changed without saying so: a 500 response records "500". The second test pins the other half, that a 404 records nothing, since the server side of the HTTP conventions does not count a 4xx as a failed request.
…-on bullet The fragments were named for pull request 21, which this one supersedes, and .github/workflows/changelog.yml rejects a fragment whose leading number differs from the number of the pull request carrying it. The fixed fragment also held three entries on three lines of one file. towncrier renders one file as one bullet regardless of how many lines it has, so those three ran together into a single unreadable entry. The regular expression in that workflow allows only one file per type per pull request, so the entries are merged into one sentence rather than split across files. urllib3 is covered by the added fragment, which now names HTTP alongside the other three domains.
…ion metric Moving the two histogram recordings to the end of the finally block, so that duration_attrs was complete before they read it, put them after the span activation exits and after the context token is detached. The span is no longer current at that point, so TraceBasedExemplarFilter, which the SDK installs by default, saw INVALID_SPAN and dropped the exemplar. Every pyramid duration measurement lost its link back to the trace, on success as well as on failure. Measured on this checkout: one exemplar before the move, none after, one again with this commit. The sibling WSGI instrumentation already had the answer, capturing the context before the span ends and passing it to record. The test is written against the exemplar list rather than the attributes, since no existing test looks at exemplars at all, which is why the loss went unnoticed.
The six database packages wrote error.type unconditionally, so a user who has not opted in, which is every user by default, got a span carrying the old experimental attributes plus one stable attribute from the new conventions. Every other attribute on those same spans is already gated, and the three database packages that set error.type before this branch all gate it: asyncpg, aiopg and pymongo. The urllib3 commit in this branch gates it too, for the HTTP conventions, so the branch disagreed with itself. _record_error_type already accepts a span of None, so the call sites that wrap a call pass the span only when the mode asks for it, rather than growing a conditional. sqlalchemy is the exception: its error handler is a module-level function with no access to the integration, so the decision is stored on the execution context next to the span it already stores there. The tests that asserted the attribute move to the opt-in mode each package already uses for its other convention tests, and redis gains a default-mode test asserting the attribute is absent. Two test classes reset the stability singleton in tearDown now, because it is a process-wide cache that the existing mode tests were already leaking into whatever ran after them; without that reset the new tests made the leak visible.
…Error Adding error.type assertions to the four async client error tests and the two async server abort tests, which this branch had changed without covering, immediately found a defect in the async server. The interceptor catches an exception from the servicer and skips the ones that are a bare Exception, because the comment there explains those are gRPC aborts the context wrapper has already handled. In grpc.aio an abort does not raise a bare Exception, it raises AbortError, so it passed that guard and the error.type written by the wrapper was replaced with the string "AbortError". The sync server records the status code name for the same call, so the two halves disagreed, and AbortError is an implementation detail of grpc.aio rather than a value the conventions allow. The guard only covers the error.type decision, leaving record_exception reached by exactly the same exceptions as before. The abort test using FAILED_PRECONDITION asserts the attribute is absent, since the server side of the gRPC conventions lists only six status codes as errors and that is not one of them. Reverting the five gRPC source files now fails eight tests rather than three.
…h the helper _set_error_type takes a metric attributes mapping precisely so that a caller writing the attribute to a span and to a duration metric cannot write one and forget the other, and no caller was passing it. pyramid is the one place in this branch whose shape matches: it has both destinations in hand at the same point. Routing it through the helper removes the duplicated type(...).__qualname__ and the is_recording guard, and leaves the parameter with a caller that justifies it.
…ts expect The commit that added the async gRPC tests claimed this source change and did not contain it, so the branch tip failed two of its own tests. The change is reconstructed here unchanged. In grpc.aio an abort raises AbortError rather than the bare Exception the sync server raises, so it passed the guard meant to skip aborts the servicer-context wrapper has already reported, and the error.type written there was replaced with the string "AbortError". For an abort with FAILED_PRECONDITION, which the server side of the conventions does not count as an error, the span kept a status of Unset and gained an error.type anyway. The guard covers only the error.type decision, so record_exception is still reached by exactly the same exceptions as before.
AGENTS.md asks for parameters and return types on every signature, and the two gRPC helpers and the remoulade activation helper had none. Neither package is in the pyright include list, so nothing caught it. The gRPC signature is the one worth having: _set_grpc_error_type takes an exception and a status code that both default to None, and the annotation is what makes it visible that a caller is expected to supply one of them.
… a code grpc.RpcError is a bare marker class with no code method, so a servicer raising one made the instrumentation raise AttributeError and the original exception never reached the caller.
… in the mode that shows it In the default mode the span is never handed to the helper, so the assertion held whatever the exclusion did.
The existing negative tests exercise a successful operation, which writes no error.type in any mode, so they held whatever the gate did.
…database stable mode
…ht servicer error
Look the status code up with a guarded call rather than a callable check, which pylint reads as calling a non-callable, stream a response before failing so the generator has no unreachable yield, and move the error.type client tests into their own class to stay under the method limit.
Pull request dashboard statusWaiting on reviewers · refreshed 2026-10-30 00:04 UTC Review the latest changes. Status above doesn't look right?
|
6 tasks
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.
Description
error.typeisConditionally Requiredon a failed operation in the database, messaging, RPC and HTTP semantic conventions, on the span and on the operation duration histogram alike, and that requirement level carries a MUST. Seventeen instrumentations did not record it. This adds it to all of them.Two shared helpers in
opentelemetry-instrumentationdo the work. A package that already has an exception handler calls_set_error_typefrom inside it, and a package that has only a bare call site wraps that call site in_record_error_type.https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/recording-errors.md
Packages
All seventeen record
error.typeon a failed operation: aio-pika, aiokafka, boto3sqs, cassandra, celery, confluent-kafka, dbapi, grpc, kafka-python, pika, pymemcache, pyramid, redis, remoulade, sqlalchemy, tortoiseorm, urllib3.Four are not the plain case:
grpcrecords the gRPC status code name rather than the exception class name, as the RPC conventions ask. The client and server error sets differ, so the server records it only for the six codes the conventions list.dbapialready recorded it on the duration metric and not on the span.pyramidis the reverse, recording it on the span and not on the duration metric.redisleaves aWatchErroruntagged, since an optimistic-locking retry is not a failed operation.Behaviour changes beyond the attribute
Three packages could not record a failure at all until something else was fixed first.
aiokafkaended its producer span before awaiting the send, so the span covered only the header injection and no failure of the send could ever reach it. The span now covers the send.boto3sqsstarts one span per entry insend_message_batchand ended them all only after the batch call returned, so an exception left every one of those spans open and unended and they were never exported. They are now ended on the failure path, and an entry reported in the responseFailedlist gets its ownerror.typeand error status.remouladereceived the exception inafter_process_messageandafter_enqueueand discarded it, closing the span as though the message had succeeded. It now ends the span with an error status, while leaving an interrupted message untagged, becauseInterruptand its subclasses signal interruption rather than failure.Stability opt-in
error.typeon a database or HTTP span is a stable-convention attribute, so the span write is gated on_report_newin the eight packages where that applies: cassandra, dbapi, pymemcache, redis, sqlalchemy and tortoiseorm on the database opt-in, and pyramid and urllib3 on the HTTP opt-in. WithoutOTEL_SEMCONV_STABILITY_OPT_INthose spans are unchanged. The messaging and RPC packages are not gated, because their conventions puterror.typeoutside that opt-in.Not changed
The HTTP server packages (django, falcon, flask, tornado, aiohttp-server) are not touched. They reach the shared
_set_statushelper, which setserror.typefrom the response status code, and an unhandled handler exception still produces a 500, so they are already compliant for that case. A narrower gap remains there for a failure that produces no status code at all, but it needs per-package plumbing to avoid overwriting the status-code value and is not attempted here.Testing
Every package except tortoiseorm has a failing-operation test and a succeeding-operation test, and each was checked to fail with the source change reverted and pass with it applied.
Package suites: aio-pika 13, aiokafka 19, boto3sqs 21, cassandra 14, celery 37, confluent-kafka 18, dbapi 66, grpc 165, kafka-python 16, pika 37, pymemcache 43, pyramid 39, redis 78, remoulade 9, sqlalchemy 71, urllib3 61. The shared package runs 242.
Two gaps.
tortoiseormis changed without a test because its suite hangs on unmodified source in my environment.boto3sqshas one failing test,test_receive_message, which fails identically on unmodified source, from a botocore incompatibility under Python 3.14.Scope
The change is mechanical and the same shape in every package, which is why it is proposed together rather than one package at a time. Happy to split it per package if maintainers prefer that for review.
Type of change
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.
tox -e precommittox -e typecheck