-
Notifications
You must be signed in to change notification settings - Fork 922
DB and HTTP semantic convention stability migration for Redis instrumentation #4370
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
base: main
Are you sure you want to change the base?
Changes from all commits
02adc40
6cc6600
269949b
aadf1d2
1d4d6d5
1c7e3cc
1a033fb
fafc229
ef149b3
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 |
|---|---|---|
|
|
@@ -14,3 +14,5 @@ | |
|
|
||
|
|
||
| _instruments = ("redis >= 2.6",) | ||
|
|
||
| _semconv_status = "migration" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,13 +20,15 @@ | |
|
|
||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from opentelemetry.instrumentation._semconv import ( | ||
| _set_db_system, | ||
| _set_http_net_peer_name_client, | ||
| _set_http_peer_port_client, | ||
| ) | ||
| from opentelemetry.semconv._incubating.attributes.db_attributes import ( | ||
| DB_REDIS_DATABASE_INDEX, | ||
| DB_SYSTEM, | ||
| ) | ||
| from opentelemetry.semconv._incubating.attributes.net_attributes import ( | ||
| NET_PEER_NAME, | ||
| NET_PEER_PORT, | ||
| NET_TRANSPORT, | ||
| ) | ||
| from opentelemetry.semconv.trace import ( | ||
|
|
@@ -46,19 +48,33 @@ | |
| _FIELD_TYPES = ["NUMERIC", "TEXT", "GEO", "TAG", "VECTOR"] | ||
|
|
||
|
|
||
| def _extract_conn_attributes(conn_kwargs): | ||
| def _extract_conn_attributes( | ||
| conn_kwargs, db_sem_conv_opt_in_mode, http_sem_conv_opt_in_mode | ||
| ): | ||
| """Transform redis conn info into dict""" | ||
| attributes = { | ||
| DB_SYSTEM: DbSystemValues.REDIS.value, | ||
| } | ||
| attributes = {} | ||
| _set_db_system( | ||
| attributes, DbSystemValues.REDIS.value, db_sem_conv_opt_in_mode | ||
| ) | ||
|
|
||
| db = conn_kwargs.get("db", 0) | ||
| attributes[DB_REDIS_DATABASE_INDEX] = db | ||
|
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 |
||
| if "path" in conn_kwargs: | ||
| attributes[NET_PEER_NAME] = conn_kwargs.get("path", "") | ||
| _set_http_net_peer_name_client( | ||
| attributes, conn_kwargs.get("path", ""), http_sem_conv_opt_in_mode | ||
| ) | ||
| attributes[NET_TRANSPORT] = NetTransportValues.OTHER.value | ||
|
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
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. yeah. maybe just set if _report_old() |
||
| else: | ||
| attributes[NET_PEER_NAME] = conn_kwargs.get("host", "localhost") | ||
| attributes[NET_PEER_PORT] = conn_kwargs.get("port", 6379) | ||
| _set_http_net_peer_name_client( | ||
| attributes, | ||
| conn_kwargs.get("host", "localhost"), | ||
| http_sem_conv_opt_in_mode, | ||
| ) | ||
| _set_http_peer_port_client( | ||
| attributes, | ||
| conn_kwargs.get("port", 6379), | ||
| http_sem_conv_opt_in_mode, | ||
| ) | ||
| attributes[NET_TRANSPORT] = NetTransportValues.IP_TCP.value | ||
|
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. Same as above. |
||
|
|
||
| return attributes | ||
|
|
@@ -99,12 +115,17 @@ def _value_or_none(values, n): | |
|
|
||
|
|
||
| def _set_connection_attributes( | ||
| span: Span, conn: RedisInstance | AsyncRedisInstance | ||
| span: Span, | ||
| conn: RedisInstance | AsyncRedisInstance, | ||
| db_sem_conv_opt_in_mode, | ||
| http_sem_conv_opt_in_mode, | ||
| ) -> None: | ||
| if not span.is_recording() or not hasattr(conn, "connection_pool"): | ||
| return | ||
| for key, value in _extract_conn_attributes( | ||
| conn.connection_pool.connection_kwargs | ||
| conn.connection_pool.connection_kwargs, | ||
| db_sem_conv_opt_in_mode, | ||
| http_sem_conv_opt_in_mode, | ||
| ).items(): | ||
| span.set_attribute(key, value) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ packaging==24.0 | |
| pluggy==1.6.0 | ||
| py-cpuinfo==9.0.0 | ||
| pytest==7.4.4 | ||
| pytest-asyncio==0.23.5 | ||
|
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. Added in 1c7e3cc so Python 3.9 tests pass, else |
||
| redis==5.0.1 | ||
| tomli==2.0.1 | ||
| typing_extensions==4.12.2 | ||
|
|
||
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.
These are duplicated across all four factories, should we add a helper?