Skip to content

[db] Add docs for context info propagation #2236

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

Closed
wants to merge 24 commits into from
Closed

[db] Add docs for context info propagation #2236

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
1529979
[db] Add docs for context info propagation
May 6, 2025
ab6e386
Update docs/database/database-spans.md
sincejune May 7, 2025
08b02a8
Update docs/database/sql-server.md
sincejune May 7, 2025
2c26648
Merge branch 'main' into db-propagate-context-info
May 7, 2025
0ccfda5
feedback
May 7, 2025
a6e65f4
feedback
May 7, 2025
886aa37
feedback
May 7, 2025
f633fa2
feedback
May 7, 2025
5b835f8
Update docs/database/database-spans.md
sincejune May 7, 2025
e021083
Update docs/database/database-spans.md
sincejune May 7, 2025
1bcbf0d
feedback
May 7, 2025
e6803f5
Update docs/database/database-spans.md
sincejune May 8, 2025
f986ee2
add single quotes
May 8, 2025
8794c1d
Add co-author
XSAM May 13, 2025
ec00e3c
feedback
May 19, 2025
fa6cf6d
fix format
May 19, 2025
2c0e01a
update baggage.service.name example
May 19, 2025
430957b
Keep only `service.name` for general databases
XSAM May 21, 2025
7f05f5e
Update sql-server doc
XSAM May 21, 2025
5534272
Adjust font format
XSAM May 21, 2025
9876582
Add an example for sql server
XSAM May 21, 2025
89a4baf
Merge remote-tracking branch 'upstream/main' into db-propagate-contex…
XSAM May 21, 2025
0e7ff53
Fix CI
XSAM May 21, 2025
8effebc
Merge remote-tracking branch 'upstream/main' into db-propagate-contex…
XSAM May 21, 2025
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
2 changes: 1 addition & 1 deletion docs/database/database-spans.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ relatively short and its cardinality remains low comparing to the `db.query.text

### SQL commenter

Instrumentations SHOULD propagate the context information to the SQL queries following [SQL commenter](https://google.github.io/sqlcommenter/spec/). The instrumentation implementation MAY choose to either `append` the comment to the end of the query or `prepend` the comment at the beginning of the query, depending on the specific database system's requirements or preferences.
Instrumentations SHOULD propagate the context information to the SQL queries following [SQL commenter](https://google.github.io/sqlcommenter/spec/). The instrumentation implementation MAY choose to either **append** the comment to the end of the query or **prepend** the comment at the beginning of the query, depending on the specific database system's requirements or preferences.
Copy link
Member

Choose a reason for hiding this comment

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

If the parameter isn't set, would it make sense to prepend by default and append if we detect a hint in Postgres (/*+)?

Copy link
Member

@XSAM XSAM May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we will need a following discussion of #2236 (comment).

I did a small experiment on postgres 17.5 by running two long queries with different comments style more than 1024 bytes.

For instance:

  • For prepend comment, I ran /*foo='content longer than 1024 bytes...'*/ SELECT state,query FROM pg_stat_activity;
  • For append comment, I ran SELECT state,query FROM pg_stat_activity /*foo='content longer than 1024 bytes...'*/;

The result I got from pg_stat_activity shows both queries get truncated.

  • For prepend comment, I got /*foo='content longer than 1024 bytes...
    • Only the truncated comment is left.
  • For append comment, I got SELECT state,query FROM pg_stat_activity /*foo='content longer than 1024 bytes...
    • Query remains untouched, as it is very short, but comment is also truncated.

So, postgres seems truncate anything more than 1024 bytes on pg_stat_activity whatever it is the query statement or comments. That means no matter the comments, a query that is more 1024 bytes is going to be truncated.

Comparing these two approaches

prepend

Pros:

  • Always keep the comments. Propagated info won't be lost.

Cons:

  • When truncating happens, the query statement get truncated.
    • Users won't see a complete query, and the correlation based on query text may not work.
  • It breaks pg_hint_plan functionality [db] Add docs for context info propagation #2236 (comment). So the instrumentation needs to actively detect a hint. (though I was not able to reproduce the issue, the hint plans I got are always the same)

append

Pros:

  • Original query is kept (assuming the query itself won't be truncated)

Cons:

  • Comments might be truncated. Propagated info will be lost.

In short, assuming prepend can break pg_hint_plan while append won't. append seems better for easy implementation. About the truncate part, we could use ALTER SYSTEM SET track_activity_query_size = 16384; to increase the limit (though it needs a restart to be effective)

Copy link
Member

Choose a reason for hiding this comment

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

@trask Do you think it is ok to take this as non-blocker to this PR? we can leave this discussion to following issue/development.

Copy link
Member

Choose a reason for hiding this comment

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

@lmolkova PTAL, is this PR on a good shape?

Copy link
Member

@lmolkova lmolkova Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I do not agree with this statement:

Instrumentations SHOULD propagate the context information to the SQL queries following SQL commenter.

  1. recommending instrumentations to modify SQL queries by default is dangerous and questionable
  2. it ruins prepared statements caches for most database systems

We can document other context propagation mechanisms (like there is something for MS SQL and postgres) for these specific databases.
It'd be ok-ish with having it opt-in for other instrumentations.

Copy link
Member

Choose a reason for hiding this comment

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

It'd be ok-ish with having it opt-in for other instrumentation.

Is it ok to doc Instrumentations MAY propagate the context information to the SQL queries following [SQL commenter](https://google.github.io/sqlcommenter/spec/). Use MAY and an annotation to express that it is an opt-in behavior?

I can also move it to MS SQL if you prefer, though I want to have a unified way to propagate context information.


| Attribute | Type | Description | Examples | Require level | Stability |
|------------------------|--------|---------------------------------------|----------|-------------------|----------------------------------------------------------------|
Expand Down
23 changes: 22 additions & 1 deletion docs/database/sql-server.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,35 @@ and SHOULD be provided **at span creation time** (if provided at all):

### SQL commenter

Instrumentations SHOULD `append` the comment to the end of the query.
Instrumentations SHOULD **append** the comment to the end of the query.
Copy link
Member

Choose a reason for hiding this comment

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

why position is important? what MS SQL instrumentation should put into the comment? in which format?

Copy link
Member

Choose a reason for hiding this comment

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

why position is important?

In Postgres, prepended comments can break query hints, as shown here. Appended comments may be truncated in long queries. Proposal: use prepended comments by default, except for Postgres.

what MS SQL instrumentation should put into the comment?

To transport static tags, such as service and peer.service, because they don't fit in context_info.

Copy link
Member

@XSAM XSAM Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

in which format?

Examples:

append: SELECT * FROM songs /*service.name='shoppingcart'*/
prepend: /*service.name='shoppingcart'*/ SELECT * FROM songs

Another discussion: #2236 (comment)


### SET CONTEXT_INFO

Instrumentations MAY make use of [SET CONTEXT_INFO](https://learn.microsoft.com/en-us/sql/t-sql/statements/set-context-info-transact-sql?view=sql-server-ver16) to add text format of [`traceparent`](https://www.w3.org/TR/trace-context/#traceparent-header) before executing a query. This is an opt-in behavior that should be explicitly enabled by the user.
Copy link
Member

Choose a reason for hiding this comment

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

what's about tracestate, baggage and other possible headers?
context injection should be propagator concern, not instrumentation one

Copy link
Member

Choose a reason for hiding this comment

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

context injection should be propagator concern, not instrumentation one

I agree if SQL has a header concept (A list of key-value pairs), so it can have a dynamic space to put tracestate and baggage. We can also have a variety of propagators.

But, SQL does not have such a concept. And CONTEXT_INFO is the binary format in SQL Server, with a maximum size of 128 bytes. It is not feasible to assign more values due to this length limitation. Even if there is no limitation, I don't know what existing format we can use to put a list into a binary value.

So, I propose to use the value traceparent, a static value, for SQL Server.


`SET CONTEXT_INFO` must be executed on the same physical connection as the SQL statement (or reuse its transaction).

Note that `SET CONTEXT_INFO` requires binary input according to its syntax: `SET CONTEXT_INFO { binary_str | @binary_var }` and has a maximum size of 128 bytes.

Example:

For a query `SELECT * FROM songs` where `traceparent` is `00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01`:

Run the following command on the same physical connection as the SQL statement:

```sql
-- The binary conversion may be done by the application or the driver.
DECLARE @BinVar varbinary(55);
SET @BinVar = CAST('00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01' AS varbinary(55));
SET CONTEXT_INFO @BinVar;
```

Then run the query:

```sql
SELECT * FROM songs;
```

## Metrics

Microsoft SQL Server client instrumentations SHOULD collect metrics according to the general
Expand Down
Loading