Db semantic conventions update - #48979
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate SQL Server compatibility issues remain unresolved.
Pull request overview
Updates Azure Monitor OpenTelemetry database semantic-convention handling while retaining deprecated attribute compatibility.
Changes:
- Adds stable database attribute support for exporter and QuickPulse telemetry.
- Updates namespace, target, and port resolution.
- Adds regression tests and changelog documentation.
File summaries
| File | Summary |
|---|---|
sdk/monitor/azure-monitor-opentelemetry-exporter/tests/trace/test_trace.py |
Adds stable database span conversion tests. |
sdk/monitor/azure-monitor-opentelemetry-exporter/tests/quickpulse/test_types.py |
Adds stable QuickPulse database tests. |
sdk/monitor/azure-monitor-opentelemetry-exporter/CHANGELOG.md |
Documents the semantic-convention update. |
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py |
Supports stable attributes; needs stable SQL Server default-port handling. |
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py |
Maps stable database attributes; needs stable SQL Server normalization. |
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/_quickpulse/_types.py |
Adds stable database mapping for QuickPulse. |
Review details
Suppressed comments (2)
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py:430
- The stable
db.system.namevalue for SQL Server ismicrosoft.sql_server, but this path only treats the deprecatedmssqlvalue as SQL via_is_sql_db. Stable SQL Server spans therefore fall through todata.type = db_systemand are emitted asmicrosoft.sql_server, instead of preserving the existingSQLdependency type produced for legacy SQL Server spans. Normalize the stable alias as well.
db_system = span.attributes.get(DB_SYSTEM_NAME) or span.attributes.get(SpanAttributes.DB_SYSTEM)
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py:167
- The stable
db.system.namevalue for SQL Server ismicrosoft.sql_server, but_get_default_port_dbonly recognizes the deprecatedmssqlvalue. A CLIENT span with the stable attribute,net.peer.name, and the default port 1433 will therefore be emitted withtarget=host:1433instead of omitting the default port, unlike the legacy path. Add the stable value to the default-port mapping and cover this case.
if port != _get_default_port_http(attributes) and port != _get_default_port_db(str(db_system_for_port)):
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
a91b022 to
95e432a
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
95e432a to
d5c7046
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Stable SQL Server normalization and server endpoint/default-port handling require fixes and regression coverage.
Review details
Suppressed comments (4)
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py:430
- The stable convention uses
db.system.name="microsoft.sql_server", but the existing type mapping and_is_sql_db/DbSystemValues.MSSQLchecks recognize only the legacymssqlvalue. A stable SQL Server span will therefore be exported with dependency typemicrosoft.sql_serverinstead of the existingSQLmapping. Normalize this stable value before the mapping or update the shared SQL-system handling, and add a regression test.
db_system = span.attributes.get(DB_SYSTEM_NAME) or span.attributes.get(SpanAttributes.DB_SYSTEM)
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py:167
- This new branch changes whether
net.peer.portis included in the target for stable database spans, but the added tests only cover the system/query/namespace/operation fields and never exercise default versus non-default database ports. A stable PostgreSQL span withnet.peer.port=5432must not becomehost:5432, while a non-default port must remain visible; add regression cases for both so this changed target-formatting path is protected.
db_system_for_port = attributes.get(db_attributes.DB_SYSTEM_NAME) or attributes.get(
SpanAttributes.DB_SYSTEM
)
if port != _get_default_port_http(attributes) and port != _get_default_port_db(str(db_system_for_port)):
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py:167
- Stable database client spans use
server.address/server.portfor the database endpoint, but this helper still derives the target only fromnet.peer.name/net.peer.ipandnet.peer.port. A span withdb.system.nameplusserver.addresstherefore loses the server host (and port) and falls back to onlydb.namespace/the system name in_get_target_for_db_dependency; read the stable server attributes here while retaining the deprecated peer fallback, and cover it in both exporter paths.
# Honor both the new stable `db.system.name` and the deprecated `db.system`.
db_system_for_port = attributes.get(db_attributes.DB_SYSTEM_NAME) or attributes.get(
SpanAttributes.DB_SYSTEM
)
if port != _get_default_port_http(attributes) and port != _get_default_port_db(str(db_system_for_port)):
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py:167
- The stable convention uses
db.system.name="microsoft.sql_server", while_get_default_port_dbrecognizes only the legacymssqlvalue. Consequently a stable SQL Server span using the default port 1433 will be rendered with:1433in its target instead of omitting the default port. Extend or normalize the database-port mapping for the stable value and cover this case in a test.
db_system_for_port = attributes.get(db_attributes.DB_SYSTEM_NAME) or attributes.get(
SpanAttributes.DB_SYSTEM
)
if port != _get_default_port_http(attributes) and port != _get_default_port_db(str(db_system_for_port)):
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
d5c7046 to
1c7a602
Compare
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
🟡 Changes recommended
Check stable-first db.operation.name ordering in the trace exporter and QuickPulse mapping.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
61d6884 to
adb8a57
Compare
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues were identified, and the changes include regression coverage.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
| data.result_code = str(status_code) | ||
| elif SpanAttributes.DB_SYSTEM in span.attributes: # Database | ||
| db_system = span.attributes[SpanAttributes.DB_SYSTEM] | ||
| elif DB_SYSTEM_NAME in span.attributes or SpanAttributes.DB_SYSTEM in span.attributes: # Database |
There was a problem hiding this comment.
The PR recognizes the new attribute name, but _is_sql_db and _get_default_port_db still recognize only legacy values. The migration guide: https://opentelemetry.io/docs/specs/semconv/non-normative/db-migration/#database-system-name documents renames including mssql → microsoft.sql_server, db2 → ibm.db2, oracle → oracle.db, and h2 → h2database.
Description
Fixes - #48958
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines