SQLAlchemy: db.statement inclusion of sqlcomment as opt-in #3112
Conversation
|
cc'ing @alexmojaki , feel free to comment/review! |
| self.enable_commenter | ||
| and not self.enable_attribute_commenter | ||
| ): | ||
| statement = _add_sql_comment(statement, **commenter_data) |
There was a problem hiding this comment.
Why do we need the _add_sql_comment after the set_attribute statement?
There was a problem hiding this comment.
@shalevr ! At this condition, user has enabled sqlcommenting but has not opted into "enable attribute commenter". So db.statement get written first without sqlcomment, then sqlcomment is added to the query statement before execution.
| and not self.enable_attribute_commenter | ||
| ): | ||
| statement = _add_sql_comment(statement, **commenter_data) | ||
|
|
There was a problem hiding this comment.
Line 267 and this line look the same both have self.enable_commenter=True, but on line 267, self.enable_attribute_commenter=True, while in this condition, self.enable_attribute_commenter=False. So, what's the difference? Can we call _add_sql_comment once to make it clearer
There was a problem hiding this comment.
I'll see how I can rearrange and add comments to make it more clear!
Currently L247 checks self.enable_commenter=True and L267 checks self.enable_attribute_commenter=True. If so, we _add_sql_comment first so that both db.statement attribute and executed query statement include the sqlcomment.
This next part is unclear to me too now 😅 If at L247 self.enable_commenter=False, then db.statement gets set first without sqlcomment. Then at L276 we check again?! Not sure how the tests are passing.
There was a problem hiding this comment.
@shalevr How does it look now? This time it's an if-(if-else)-else with comments.
...elemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py
Outdated
Show resolved
Hide resolved
|
|
||
| def _set_db_client_span_attributes(self, span, statement, attrs) -> None: | ||
| """Uses statement and attrs to set attributes of provided Otel span""" | ||
| span.set_attribute(SpanAttributes.DB_STATEMENT, statement) |
There was a problem hiding this comment.
Prob isn't related, but shouldn't these attributes be provided at span creation? For sampling decisions.
There was a problem hiding this comment.
If so, maybe we can pass them at span creation and convert this method to a staticmethod that just update the statement with the statement with commenter_data.
There was a problem hiding this comment.
If so, maybe we can pass them at span creation and convert this method to a staticmethod that just update the statement with the statement with commenter_data.
There was a problem hiding this comment.
The calls to set_attribute in this instrumentor generally have been set to only happen if span.is_recording(), which for this (new) helper is at L272. This condition was introduced in open-telemetry/opentelemetry-python#1057 to improve performance, so I think that part should be kept in. Then regarding staticmethod, I'm not sure if it'll be possible but maybe I miss something. Thoughts? 🙂
...elemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py
Outdated
Show resolved
Hide resolved
...elemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py
Outdated
Show resolved
Hide resolved
...elemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| if self.commenter_options.get("opentelemetry_values", True): | ||
| commenter_data.update(**_get_opentelemetry_values()) |
There was a problem hiding this comment.
Related to this comment it seems strange to have trace context related information in a span attribute. I am okay with sqlcommenting as part of the attribute but maybe we can omit the OT values?
There was a problem hiding this comment.
Capturing Otel context in db.statement is a convenience for checking that sqlcommenter is enabled with the current config, and checking that the expected trace context was injected into the query statement sent from db client app to to the db server. The latter is an instrumentation checkup; duplicate traceparent can be sqlcommented if both SQLAlchemy (ORM) and db driver (e.g. pymysql/psycopg2) are enabled unintentionally and this could mess with the client-server correlation.
It's not a requirement since all can both be checked in the mysql/postgresql server general logs. But diagnosing issues is quicker with db.statement inclusion of full sqlcomment, e.g. looking at span attributes in a local otel-collector exporting to console versus accessing a mysql server and opening its logs. Or e.g. looking at spans exported to a platform versus getting creds and accessing a cloud-hosted db server.
…pentelemetry/instrumentation/sqlalchemy/__init__.py Co-authored-by: Leighton Chen <[email protected]>
…pentelemetry/instrumentation/sqlalchemy/__init__.py Co-authored-by: Leighton Chen <[email protected]>
Description
Supports new, optional kwarg
enable_attribute_commenterat SQLAlchemy instrumentation to opt intodb.statementspan attribute inclusion of sqlcomment.Partially addresses #3107
Type of change
Breaking because
db.statementspan attribute inclusion of sqlcomment currently always happens if sqlcommenter enabled. It'll be opt-in with this update, so might break for those users relying on the feature introduced in 0.50b0.How Has This Been Tested?
tox -e py312-test-instrumentation-sqlalchemy-0tox -e py312-test-instrumentation-sqlalchemy-1tox -e py312-test-instrumentation-sqlalchemy-2enable_attribute_commenter=Truevs unsetDoes This PR Require a Core Repo Change?
Checklist: