-
Notifications
You must be signed in to change notification settings - Fork 276
[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.
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
Changes from all commits
1529979
ab6e386
08b02a8
2c26648
0ccfda5
a6e65f4
886aa37
f633fa2
5b835f8
e021083
1bcbf0d
e6803f5
f986ee2
8794c1d
ec00e3c
fa6cf6d
2c0e01a
430957b
7f05f5e
5534272
9876582
89a4baf
0e7ff53
8effebc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| # Use this changelog template to create an entry for release notes. | ||
| # | ||
| # If your change doesn't affect end users you should instead start | ||
| # your pull request title with [chore] or use the "Skip Changelog" label. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: enhancement | ||
|
|
||
| # The name of the area of concern in the attributes-registry, (e.g. http, cloud, db) | ||
| component: db | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: Add doc for context propagation | ||
|
|
||
| # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
| # The values here must be integers. | ||
| issues: [2162] | ||
|
|
||
| # (Optional) One or more lines of additional information to render under the primary note. | ||
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,8 @@ linkTitle: Spans | |
| - [Notes and well-known identifiers for `db.system.name`](#notes-and-well-known-identifiers-for-dbsystemname) | ||
| - [Sanitization of `db.query.text`](#sanitization-of-dbquerytext) | ||
| - [Generating a summary of the query](#generating-a-summary-of-the-query) | ||
| - [Context Propagation](#context-propagation) | ||
| - [SQL commenter](#sql-commenter) | ||
| - [Semantic conventions for specific database technologies](#semantic-conventions-for-specific-database-technologies) | ||
|
|
||
| <!-- tocstop --> | ||
|
|
@@ -478,6 +480,28 @@ Semantic conventions for individual database systems or specialized instrumentat | |
| MAY specify a different `db.query.summary` format as long as produced summary remains | ||
| relatively short and its cardinality remains low comparing to the `db.query.text`. | ||
|
|
||
| ## Context Propagation | ||
|
|
||
| **Status**: [Development][DocumentStatus] | ||
|
|
||
| ### 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. | ||
|
|
||
| | Attribute | Type | Description | Examples | Require level | Stability | | ||
| |------------------------|--------|---------------------------------------|----------|-------------------|----------------------------------------------------------------| | ||
| | [`service.name`](/docs/registry/attributes/service.md) | string | Logical name of the service [22] | `shoppingcart` | `Recommended` |  | | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this table talks about attributes, but it's under context propagation - I don't understand the suggestion here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That means when using sqlcommenter for context propagation, we should propagate The reason is here. #2236 (comment). This is a tradeoff because the database itself won't provide trace info. We can use database receiver sending SQL to fetch query samples with trace info, but the chance of getting a complete trace (from SQL client to SQL server) is low due to sampling. Do you think suggesting a |
||
|
|
||
| **[22] `service.name`:** Instrumentations MAY use [SDK-provided resource detectors](https://opentelemetry.io/docs/specs/semconv/resource/#semantic-attributes-with-sdk-provided-default-value) to set the default value for this attribute. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instrumentations don't have access to resources - resources are SDK concept and instrumentations only use API There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I forgot this part. Sounds like |
||
|
|
||
| **Examples:** | ||
|
|
||
| - For a query `SELECT * FROM songs` where `service.name` is `shoppingcart`: | ||
|
|
||
| ```sql | ||
| SELECT * FROM songs /*service.name='shoppingcart'*/ | ||
| ``` | ||
|
|
||
| ## Semantic conventions for specific database technologies | ||
|
|
||
| More specific Semantic Conventions are defined for the following database technologies: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,9 @@ linkTitle: SQL Server | |
| <!-- toc --> | ||
|
|
||
| - [Spans](#spans) | ||
| - [Context Propagation](#context-propagation) | ||
| - [SQL commenter](#sql-commenter) | ||
| - [SET CONTEXT_INFO](#set-context_info) | ||
| - [Metrics](#metrics) | ||
|
|
||
| <!-- tocstop --> | ||
|
|
@@ -152,6 +155,41 @@ and SHOULD be provided **at span creation time** (if provided at all): | |
| <!-- END AUTOGENERATED TEXT --> | ||
| <!-- endsemconv --> | ||
|
|
||
| ## Context Propagation | ||
|
|
||
| **Status**: [Development][DocumentStatus] | ||
|
|
||
| ### SQL commenter | ||
|
|
||
| Instrumentations SHOULD **append** the comment to the end of the query. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
To transport static tags, such as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Examples: append: 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. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's about tracestate, baggage and other possible headers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree if SQL has a header concept (A list of key-value pairs), so it can have a dynamic space to put But, SQL does not have such a concept. And So, I propose to use the value |
||
|
|
||
| `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 | ||
|
|
||
There was a problem hiding this comment.
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 (
/*+)?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.5by running two long queries with different comments style more than 1024 bytes.For instance:
/*foo='content longer than 1024 bytes...'*/ SELECT state,query FROM pg_stat_activity;SELECT state,query FROM pg_stat_activity /*foo='content longer than 1024 bytes...'*/;The result I got from
pg_stat_activityshows both queries get truncated./*foo='content longer than 1024 bytes...SELECT state,query FROM pg_stat_activity /*foo='content longer than 1024 bytes...So, postgres seems truncate anything more than 1024 bytes on
pg_stat_activitywhatever 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:
Cons:
append
Pros:
Cons:
In short, assuming prepend can break
pg_hint_planwhile append won't. append seems better for easy implementation. About the truncate part, we could useALTER SYSTEM SET track_activity_query_size = 16384;to increase the limit (though it needs a restart to be effective)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok to doc
Instrumentations MAY propagate the context information to the SQL queries following [SQL commenter](https://google.github.io/sqlcommenter/spec/).UseMAYand 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.