Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convention for cancelled spans #560

Open
charleskorn opened this issue Nov 24, 2023 · 5 comments
Open

Convention for cancelled spans #560

charleskorn opened this issue Nov 24, 2023 · 5 comments
Assignees

Comments

@charleskorn
Copy link

It would be great if there was a language- and protocol-agnostic convention for marking a span as representing a cancelled operation.

At present, different languages and protocols treat cancelled operations in different ways, which makes it difficult to do things like filter a trace to show only non-cancelled operations, or visually distinguish between successful, failed and cancelled operations.

For example, a span representing a gRPC call would have the rpc.grpc.status_code attribute set to 1 (for the gRPC CANCELLED status code), while a span representing a Golang operation might be recorded with an event with a context canceled message.

Instead, it'd be great if there was a convention for an attribute that marked a span as cancelled, such as cancelled.

@trask
Copy link
Member

trask commented Nov 28, 2023

oh no, we'll need to make a decision on canceled vs cancelled 😅

in seriousness, a standard "canceled" (or "cancelled") attribute would be very welcome in opentelemetry-java-instrumentation repo where we currently have (at least):

  • guava.canceled
  • rxjava.canceled
  • jaxrs.canceled
  • reactor.canceled
  • grpc.canceled

@mateuszrzeszutek
Copy link
Member

Didn't we want to use the error.type attribute for that? At least in the HTTP scenarios, e.g. error.type = cancelled

@pyohannes
Copy link
Contributor

Didn't we want to use the error.type attribute for that? At least in the HTTP scenarios, e.g. error.type = cancelled

The problem with using error.type is, that a cancelled operation doesn't necessarily imply an error. However, the definition of error.type states this:

If the operation has completed successfully, instrumentations SHOULD NOT set error.type.

@lmolkova
Copy link
Contributor

lmolkova commented Dec 11, 2023

I'd be in favor of defining something in common.

I think we can polish error.type description to accommodate it.

If the operation has completed successfully, instrumentations SHOULD NOT set error.type.

  • cancelled operation did not complete successfully and it should be totally legit to use error.type for it in the same way as we set error.type to "404" on the client side even when 404 does not represent an error.

Some problem with the cancel(l)ed convention:

  • E.g. on Java the async operations (like reactor ones) can be cancelled, but sync versions of them throw something like TimeoutException or InterruptedException - I'm not sure how far we can go unifying and if by replacing exception type with cancelled is helpful.
  • There were some concerns about attempts to unify across languages. E.g. in .NET cancellation is commonly done with OperationCancelledException that's familiar to all .NET devs - what should .net instrumentations do?

The least controversial option I can suggest is to use cancel(l)ed when operation is cancelled without exception.

@pyohannes
Copy link
Contributor

cancelled operation did not complete successfully and it should be totally legit to use error.type

That depends very much on the context. In some cases, for example if I decide that I don't need the output of an operation anymore, I cancel an operation and cancellation is the desired outcome. I don't want this call to count against failed calls in my downstream calls.

Also, let's make clear whether we are talking about setting error.type to canceled on spans, on metrics, or on both. For spans, would we additionally set the span status to ERROR for canceled operations?

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

No branches or pull requests

7 participants