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.

Sign up for GitHub

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

Jump to bottom

Add section on span status for databases #1560

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+57 −0
Open

Add section on span status for databases #1560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8c57ad
Add section on span status for databases
lmolkova Nov 8, 2024
7cd5ed5
Apply suggestions from code review
lmolkova Nov 8, 2024
566bf08
review
lmolkova Nov 8, 2024
964fdfd
changelog and nits
lmolkova Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .chloggen/1560.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
change_type: enhancement
component: db
note: Specify how to set span kind for database operations.
issues: [1536, 1560]
53 changes: 53 additions & 0 deletions docs/database/database-spans.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ linkTitle: Client Calls
<!-- toc -->

- [Name](#name)
- [Status](#status)
- [Recording exception events](#recording-exception-events)
- [Common attributes](#common-attributes)
- [Notes and well-known identifiers for `db.system`](#notes-and-well-known-identifiers-for-dbsystem)
- [Sanitization of `db.query.text`](#sanitization-of-dbquerytext)
Expand Down Expand Up @@ -85,6 +87,56 @@ and SHOULD adhere to one of the following values, provided they are accessible:
If a corresponding `{target}` value is not available for a specific operation, the instrumentation SHOULD omit the `{target}`.
For example, for an operation describing SQL query on an anonymous table like `SELECT * FROM (SELECT * FROM table) t`, span name should be `SELECT`.

## Status

[Span Status Code][SpanStatus] MUST be left unset if the operation has ended without any errors.

Instrumentation SHOULD consider the operation as failed if any of the following is true:

- the `db.response.status_code` value indicates an error

> [!NOTE]
> The classification of status code as an error depends on the context.
> For example, a SQL STATE `02000` (`no_data`) indicates an error when the application
> expected the data to be available. However, it is not an error when the
> application is simply checking whether the data exists.
>
> Instrumentations that have additional context about a specific operation MAY use
> this context to set the span status more precisely.
> Instrumentations that don't have any additional context MUST follow the
> guidelines in this section.

- an exception is thrown by the instrumented method call
- the instrumented method returns an error in another way

When the operation ends with an error, instrumentation:

- SHOULD set the span status code to `Error`
- SHOULD set the `error.type` attribute
- SHOULD set the span status description when it has additional information
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this have any requirements about not having any confidential data? Similar to how we have on metrics errors where we don't add if it shows actual values of queries, etc

about the error that aligns with [Span Status Description][SpanStatus].

It's NOT RECOMMENDED to duplicate `db.response.status_code` or `error.type`
in span status description.

When the operation fails with an exception, the span status description SHOULD be set to
the exception message.

### Recording exception events

**Status**: [Experimental][DocumentStatus]

When the operation fails with an exception, instrumentation SHOULD record
an [exception event](../exceptions/exceptions-spans.md) by default if and only if the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
an [exception event](../exceptions/exceptions-spans.md) by default if and only if the
an [exception event](../exceptions/exceptions-spans.md) by default if, and only if, the

span being recorded is a local root span (does not have a local parent).

> [!NOTE]
> Exception stack traces could be very long and are expensive to capture and store.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> Exception stack traces could be very long and are expensive to capture and store.
>
> Exception stack traces could be very long and are expensive to capture and store.

I think you need an empty line here so the icon shows up.

> Exceptions which are not handled by instrumented libraries are likely to be handled
> and logged by the caller.
> Exceptions that are not handled will be recorded by the outermost (local root)
> instrumentation such as HTTP or gRPC server.
Comment on lines +129 to +138
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's ask for feedback on this point from the Specification SIG before we merge

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a directional shift in logging exceptions.

I see two things here:

  1. We still can't determine "layers" of instrumentation, i.e. we can't know if someone else will log the exception or not.
  2. This violates the previously recorded wisdom of how to handle exceptions and stack traces.

My opinion:

This is the right direction to go. We should assume that there is "outer" instrumentation that logs/catches exceptions, particularly for database "client" spans. I.e. we may be able to blanketly state that "client" spans should never log unhandled exceptions and that we encourage "outer" instrumentation that will (e.g. server spans).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added to next week's spec SIG agenda


## Common attributes

These attributes will usually be the same for all operations performed over the same database connection.
Expand Down Expand Up @@ -422,3 +474,4 @@ More specific Semantic Conventions are defined for the following database techno
* [SQL](sql.md): Semantic Conventions for *SQL* databases.

[DocumentStatus]: https://opentelemetry.io/docs/specs/otel/document-status
[SpanStatus]: https://github.com/open-telemetry/opentelemetry-specification/tree/v1.37.0/specification/trace/api.md#set-status
Loading