dbapi: Add db.collection.name, use connection kwargs for connection attributes #2869
Conversation
|
|
|
I'm working on getting the CLA signed, but for what it's worth I've passed dbapi tests and linting locally |
|
Making progress on the CLA, I've got a person who said they'd be the CLA manager |
…try-python-contrib into dbapi-more-attributes
|
We've got a CLA manager, just awaiting approval now |
|
Sweet, I just got approved. Good to review now I believe |
|
@emdneto Do you know who I could ping to get a review on this? |
…try-python-contrib into dbapi-more-attributes
There was a problem hiding this comment.
Thanks for the PR.
Since the semantic conventions for Database Calls are Experimental, I understand we shouldn't change the version of the database conventions that we emit until it's marked as stable. This implies not adding new attributes that only exist in the new versions of the semantic conventions.
However, I'm ok with adding the DB name to the span name because in v1.11.0 it is allowed.
You may want to consider addressing the linting issues and setting up pre-commit in your environment.
|
@emdneto Linting and test failures addressed: I ran the I've run |
…try-python-contrib into dbapi-more-attributes
|
I'm having some trouble running the |
| @@ -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.
I'm having some trouble running the docker-tests on my machine. Before I dive into it @emdneto , I just wanted to make sure that the failing test isn't flaky as far as you know?
@keithZmudzinski, they are legit tests that need to be fixed if we want this PR to be merged.
| 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.
But as I said in the comments before, I wouldn't introduce new semantic convention attributes in this PR.
| # First set from kwargs | ||
| if kwargs and value in kwargs: | ||
| self.connection_props[key] = kwargs.get(value) |
There was a problem hiding this comment.
Thanks for putting in this PR!
- Uses more sources for the db.user, net.peer.name, and net.peer.port span attributes.
Could some tests be added to show expectations for this?
| return " ".join( | ||
| name for name in (operation_name, collection_name) if name | ||
| ) |
There was a problem hiding this comment.
Do these changes also implement these parts of the spec?
If there is no (low-cardinality) db.operation.name available, database span names SHOULD be {target}.
If neither {db.operation.name} nor {target} are available, span name SHOULD be {db.system}.
I think Yes for the first point, would be good to see some test cases for that as well.
|
Hi @keithZmudzinski, we talked about Otel database semantic conventions in today's Python SIG meeting. The changes in PR are appreciated and will be needed at some point. However, because the database semconv is not yet stable (currently it's release candidate), we will wait before making the proposed updates for the span name and attributes changes. This is a topic we will be bringing up with the Otel TC for guidance. |
|
@tammy-baylis-swi Thanks for the update, and for the feedback, I really appreciate it. Should I let this PR stay put, or close it out and open a new one when the database conventions are marked as STABLE? |
|
@keithZmudzinski I'd say it's easiest to Close this one and start a fresh PR later, since this monorepo tends to get a lot of merge conflicts introduced. |
Description
This pull request adds 3 features to the opentelemetry-instrumentation-dbapi library.
db.collection.namein the span name if possible. This is in line with the semantic conventions for database span names: https://opentelemetry.io/docs/specs/semconv/database/database-spans/#namedb.collection.nameattribute to the span if possible.db.user,net.peer.name, andnet.peer.portspan attributes.MySQLdb, there wasn't a way to get the host from the instantiated Connection class. However, the host is present in the kwargs used to create the connection. If we parse the kwargs for these attributes, we are more likely to get them.Type of change
How Has This Been Tested?
I've used these changes in a local project using the console exporter and New Relic across a variety of sql connections. Confirmed the expected attributes were populated.
Added unit tests to verify the changes.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.