-
Notifications
You must be signed in to change notification settings - Fork 606
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
dbapi: Add db.collection.name, use connection kwargs for connection attributes #2869
Changes from all commits
a737fc7
b54bbca
f2971f4
e32234e
95ed7bb
38029b5
f70c613
ecab5a3
ba0466a
038cf47
1b78b97
09d761d
fbc0faf
93f13bf
65628c4
a9ed2b5
68095ef
a8273b6
192d202
0329dad
6e9117d
b2b6849
4df2fc4
ef5719b
2b8cdea
cbe2aae
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 |
---|---|---|
|
@@ -51,6 +51,9 @@ | |
_get_opentelemetry_values, | ||
unwrap, | ||
) | ||
from opentelemetry.semconv._incubating.attributes.db_attributes import ( | ||
DB_COLLECTION_NAME, | ||
) | ||
from opentelemetry.semconv.trace import SpanAttributes | ||
from opentelemetry.trace import SpanKind, TracerProvider, get_tracer | ||
|
||
|
@@ -217,7 +220,7 @@ def instrument_connection( | |
enable_commenter=enable_commenter, | ||
commenter_options=commenter_options, | ||
) | ||
db_integration.get_connection_attributes(connection) | ||
db_integration.get_connection_attributes(connection=connection) | ||
return get_traced_connection_proxy(connection, db_integration) | ||
|
||
|
||
|
@@ -284,12 +287,17 @@ def wrapped_connection( | |
): | ||
"""Add object proxy to connection object.""" | ||
connection = connect_method(*args, **kwargs) | ||
self.get_connection_attributes(connection) | ||
self.get_connection_attributes(connection=connection, kwargs=kwargs) | ||
return get_traced_connection_proxy(connection, self) | ||
|
||
def get_connection_attributes(self, connection): | ||
# Populate span fields using connection | ||
def get_connection_attributes(self, connection, kwargs=None): | ||
# Populate span fields using kwargs and connection | ||
for key, value in self.connection_attributes.items(): | ||
# First set from kwargs | ||
if kwargs and value in kwargs: | ||
self.connection_props[key] = kwargs.get(value) | ||
|
||
# Then override from connection object | ||
# Allow attributes nested in connection object | ||
attribute = functools.reduce( | ||
lambda attribute, attribute_value: getattr( | ||
|
@@ -373,14 +381,19 @@ def _populate_span( | |
): | ||
if not span.is_recording(): | ||
return | ||
|
||
statement = self.get_statement(cursor, args) | ||
collection_name = self.get_collection_name(statement) | ||
|
||
span.set_attribute( | ||
SpanAttributes.DB_SYSTEM, self._db_api_integration.database_system | ||
) | ||
span.set_attribute( | ||
SpanAttributes.DB_NAME, self._db_api_integration.database | ||
) | ||
span.set_attribute(SpanAttributes.DB_STATEMENT, statement) | ||
if collection_name: | ||
span.set_attribute(DB_COLLECTION_NAME, collection_name) | ||
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. But as I said in the comments before, I wouldn't introduce new semantic convention attributes in this PR. |
||
|
||
for ( | ||
attribute_key, | ||
|
@@ -391,12 +404,32 @@ def _populate_span( | |
if self._db_api_integration.capture_parameters and len(args) > 1: | ||
span.set_attribute("db.statement.parameters", str(args[1])) | ||
|
||
def get_operation_name(self, cursor, args): # pylint: disable=no-self-use | ||
def get_span_name(self, cursor, args): | ||
operation_name = self.get_operation_name(cursor, args) | ||
statement = self.get_statement(cursor, args) | ||
collection_name = CursorTracer.get_collection_name(statement) | ||
return " ".join( | ||
name for name in (operation_name, collection_name) if name | ||
) | ||
Comment on lines
+411
to
+413
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. Do these changes also implement these parts of the spec?
I think Yes for the first point, would be good to see some test cases for that as well. |
||
|
||
def get_operation_name(self, cursor, args): | ||
if args and isinstance(args[0], str): | ||
# Strip leading comments so we get the operation name. | ||
return self._leading_comment_remover.sub("", args[0]).split()[0] | ||
return "" | ||
|
||
@staticmethod | ||
def get_collection_name(statement): | ||
collection_name = "" | ||
match = re.search( | ||
r"\b(?:FROM|JOIN|INTO|UPDATE|TABLE(?: IF NOT EXISTS)?)\s+(['`]?(\w+)['`]?(?:\s*\.\s*['`]?(\w+)['`]?)?)", | ||
statement, | ||
) | ||
if match: | ||
collection_name = match.group(1) | ||
|
||
return collection_name | ||
|
||
def get_statement(self, cursor, args): # pylint: disable=no-self-use | ||
if not args: | ||
return "" | ||
|
@@ -412,7 +445,7 @@ def traced_execution( | |
*args: typing.Tuple[typing.Any, typing.Any], | ||
**kwargs: typing.Dict[typing.Any, typing.Any], | ||
): | ||
name = self.get_operation_name(cursor, args) | ||
name = self.get_span_name(cursor, args) | ||
if not name: | ||
name = ( | ||
self._db_api_integration.database | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,23 +84,23 @@ def test_execute(self): | |
stmt = "CREATE TABLE IF NOT EXISTS test (id INT)" | ||
with self._tracer.start_as_current_span("rootSpan"): | ||
self._cursor.execute(stmt) | ||
self.validate_spans("CREATE") | ||
self.validate_spans("CREATE test") | ||
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.
@keithZmudzinski, they are legit tests that need to be fixed if we want this PR to be merged. |
||
|
||
def test_execute_with_cursor_context_manager(self): | ||
"""Should create a child span for execute with cursor context""" | ||
stmt = "CREATE TABLE IF NOT EXISTS test (id INT)" | ||
with self._tracer.start_as_current_span("rootSpan"): | ||
with self._connection.cursor() as cursor: | ||
cursor.execute(stmt) | ||
self.validate_spans("CREATE") | ||
self.validate_spans("CREATE test") | ||
|
||
def test_executemany(self): | ||
"""Should create a child span for executemany""" | ||
stmt = "INSERT INTO test (id) VALUES (%s)" | ||
with self._tracer.start_as_current_span("rootSpan"): | ||
data = (("1",), ("2",), ("3",)) | ||
self._cursor.executemany(stmt, data) | ||
self.validate_spans("INSERT") | ||
self.validate_spans("INSERT test") | ||
|
||
def test_callproc(self): | ||
"""Should create a child span for callproc""" | ||
|
@@ -116,12 +116,12 @@ def test_commit(self): | |
data = (("4",), ("5",), ("6",)) | ||
self._cursor.executemany(stmt, data) | ||
self._connection.commit() | ||
self.validate_spans("INSERT") | ||
self.validate_spans("INSERT test") | ||
|
||
def test_rollback(self): | ||
stmt = "INSERT INTO test (id) VALUES (%s)" | ||
with self._tracer.start_as_current_span("rootSpan"): | ||
data = (("7",), ("8",), ("9",)) | ||
self._cursor.executemany(stmt, data) | ||
self._connection.rollback() | ||
self.validate_spans("INSERT") | ||
self.validate_spans("INSERT test") |
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.
Thanks for putting in this PR!
Could some tests be added to show expectations for this?