-
Notifications
You must be signed in to change notification settings - Fork 369
[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 5 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 information 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,7 @@ 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) | ||||
| - [Propagating context to databases](#propagating-context-to-databases) | ||||
| - [Semantic conventions for specific database technologies](#semantic-conventions-for-specific-database-technologies) | ||||
|
|
||||
| <!-- tocstop --> | ||||
|
|
@@ -478,6 +479,37 @@ 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`. | ||||
|
|
||||
| ## Propagating context to databases | ||||
|
|
||||
| **Status**: [Development][DocumentStatus] | ||||
|
|
||||
| Instrumentations SHOULD propagate the context information to the SQL queries following [sqlcommenter](https://google.github.io/sqlcommenter/spec/). | ||||
|
Contributor
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 wanted to share a learning from my org. We've implemented a sqlcommenter-like approach for our instrumentations and we found it more beneficial to prepend the sql comment rather than append to the query text. This runs counter to the sqlcommenter spec, but we found that most databases truncate the query in views like pg_stat_activity, so prepending the attributes offered a better guarantee that they wouldn't be truncated
Contributor
Author
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. @zacharycmontoya Thank you for pointing this out! I think it makes sense to prepend the sql comment instead. I'll update this PR accordingly if there are no objections. cc @open-telemetry/semconv-db-approvers @XSAM
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. Using prepend conflicts the spec of sql commenter. We might need to change the spec of sql commenter if prepend approach is superior than append approach. @zacharycmontoya Could you elaborate more about this? How serious and often the truncate will happen to certain databases if we use append.
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. Postgres limits query text to 1024 characters by default, but you can increase it.
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. Prepended comments break pg_hint_plan functionality:
Contributor
Author
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. Another idea is to make it flexible by allowing the implementation of propagation for a specific database to choose between
Contributor
Author
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. Modified this PR to incorporate the idea.
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. I'm concerned about pushing this as a standard in light of google/sqlcommenter#284
Contributor
Author
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. The plan is to use
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. this PR is still about the context propagation, right? trace-id + span-id are pretty much guaranteed to be unique
Contributor
Author
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. @lmolkova The |
||||
|
|
||||
| | Attribute | Type | Description | Require level | Stability | | ||||
|
pellared marked this conversation as resolved.
Outdated
|
||||
| |----------------|--------|---------------------------------------|-------------------|----------------------------------------------------------------| | ||||
| | `service.name` | string | Logical name of the service [1] | `Required` |  | | ||||
|
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. Why is this needed?
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. I think this is because of the limitation of actual implementation. When propagating Some questions here:
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. @XSAM, it is reasonable to me to have this attribute when traceid cannot be injected due to performance reasons.
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. Gotcha. In such case
Contributor
Author
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've updated this PR and still keep the sheet to represent the minimal data that should be propagated. Please let me know if you feel it's unnecessary.
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.
We somehow need to propagate this information. I was thinking of reusing some existing propagator, instead of creating a new one. The alternative would be probably creating a new "service name propagator". However, it feels that a baggage propagator can be used for this. From https://www.w3.org/TR/baggage/#example-use-case:
I just noticed that here is a nice doc for context propagation for AWS: https://github.com/open-telemetry/semantic-conventions/blob/45e91aedff7d347955ba09269b871f8fd9049ce3/docs/non-normative/compatibility/aws.md#context-propagation
Example:
I think that we just need to write it down as I do not think we have any semantic conventions for baggage key-values so far. CC @reyang, @dyladan, @yurishkuro, @arminru
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.
Contributor
Author
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. @pellared Do you recommend using the format
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. I suggest exploring
Contributor
Author
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. @pellared Updated the examples accordingly. |
||||
| | `traceparent` | string | The trace context of current span [2] | `Recommended` [3] |  | | ||||
|
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. Open question, not a blocker for the PR, can be considered separately. |
||||
|
|
||||
| **[1] `service.name`:** MUST be the same for all instances of horizontally scaled services. If the value was not specified, SDKs MUST fall back to `unknown_service:` concatenated with [process.executable.name](https://opentelemetry.io/docs/specs/semconv/attributes-registry/process/), e.g. `unknown_service:bash`. If `process.executable.name` is not available, the value MUST be set to `unknown_service`. | ||||
|
|
||||
| **[2] `traceparent`:** MUST be in the [text format](https://www.w3.org/TR/trace-context/#traceparent-header). | ||||
|
|
||||
| **[3] `traceparent`:** `tracparent` have extremely high-cardinality. It's RECOMMENDED to propagate this info if the high-cardinality is safe for the behind databases | ||||
|
sincejune marked this conversation as resolved.
Outdated
|
||||
|
|
||||
| **Examples:** | ||||
|
|
||||
| - Query with `service.name`: | ||||
|
|
||||
| ```sql | ||||
| SELECT * FROM songs /* service.name=music-player:play */ | ||||
| ``` | ||||
|
|
||||
| - Query with `service.name` and `traceparent` | ||||
|
|
||||
| ```sql | ||||
| SELECT * FROM songs /* service.name=music-player:play, traceparent=00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01 */ | ||||
|
pellared marked this conversation as resolved.
Outdated
|
||||
| ``` | ||||
|
|
||||
| ## 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,7 @@ linkTitle: SQL Server | |
| <!-- toc --> | ||
|
|
||
| - [Spans](#spans) | ||
| - [Propagating context to SQL Server](#propagating-context-to-sql-server) | ||
| - [Metrics](#metrics) | ||
|
|
||
| <!-- tocstop --> | ||
|
|
@@ -152,6 +153,23 @@ and SHOULD be provided **at span creation time** (if provided at all): | |
| <!-- END AUTOGENERATED TEXT --> | ||
| <!-- endsemconv --> | ||
|
|
||
| ### Propagating context to SQL Server | ||
|
sincejune marked this conversation as resolved.
Outdated
|
||
|
|
||
| **Status**: [Development][DocumentStatus] | ||
|
|
||
| Instrumentations SHOULD propagate the context information to the SQL queries following [sqlcommenter](https://google.github.io/sqlcommenter/spec/). | ||
|
|
||
| | Attribute | Type | Description | Require level | Stability | | ||
| |----------------|--------|---------------------------------------|---------------|----------------------------------------------------------------| | ||
| | `service.name` | string | Logical name of the service [1] | `Required` |  | | ||
| | `traceparent` | string | The trace context of current span [2] | `Recommended` |  | | ||
|
|
||
| **[1] `service.name`:** MUST be the same for all instances of horizontally scaled services. If the value was not specified, SDKs MUST fall back to `unknown_service:` concatenated with [process.executable.name](https://opentelemetry.io/docs/specs/semconv/attributes-registry/process/), e.g. `unknown_service:bash`. If `process.executable.name` is not available, the value MUST be set to `unknown_service`. | ||
|
|
||
| **[2] `traceparent`:** MUST be in the [text format](https://www.w3.org/TR/trace-context/#traceparent-header). | ||
|
|
||
| Instrumentations SHOULD 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 high-cardinality information(e.g. `traceparent`) as adding sql comments to queries changes their unique identifier in SQL Server. | ||
|
Contributor
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. We should add to the guidance here that instrumentations will need to copy over the transaction (if present) to the new
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. Could you elaborate more about what
Contributor
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. Here's the LOC I'm referencing for our C# client : https://github.com/DataDog/dd-trace-dotnet/pull/6233/files#diff-29ead36b0177265c725705ce75f5a441417f4c22e99f7080bc8ef0f486b723a7R145-R146 When creating a new
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.
Contributor
Author
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. Updated |
||
|
|
||
| ## Metrics | ||
|
|
||
| Microsoft SQL Server client instrumentations SHOULD collect metrics according to the general | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.