Support enable_commenter for instrument_connection by psycopg(2#3071
Support enable_commenter for instrument_connection by psycopg(2#3071tammy-baylis-swi wants to merge 30 commits intoopen-telemetry:mainfrom
Conversation
tammy-baylis-swi
force-pushed
the
enable-commenter-instrument-connection
branch
from
b102323 to
bf49c8c
Compare
github-actions
bot
assigned
codeboten,
NathanielRN,
nikosokolik,
oxeye-nikolay,
shalevr and
srprash
tammy-baylis-swi
unassigned
federicobond,
codeboten,
NathanielRN,
srprash,
shalevr,
nikosokolik and
oxeye-nikolay
tammy-baylis-swi
force-pushed
the
enable-commenter-instrument-connection
branch
from
4f969ef to
e148c9f
Compare
|
Sorry to drag in several participants automatically! I committed changes I didn't mean to, and it's fixed now. |
tammy-baylis-swi
requested review from
a team
and removed request for
federicobond
|
Hi @open-telemetry/opentelemetry-python-contrib-approvers it's been a while for this one. May I please have a review? Related PR #3043 has been approved though not yet merged. |
| trace_id = format(span.get_span_context().trace_id, "032x") | ||
| self.assertEqual( | ||
| MockCursor.execute.call_args[0][0], | ||
| f"Select 1 /*db_driver='psycopg%%3Afoobar',dbapi_level='123',dbapi_threadsafety='123',driver_paramstyle='test',libpq_version='foobaz',traceparent='00-{trace_id}-{span_id}-01'*/", |
There was a problem hiding this comment.
Can we update this to use {trace_flags:02x} instead of hard coding 01 for the trace flags since this will change with the introduction of the random trace id flag. Context: open-telemetry/opentelemetry-python#4854
There was a problem hiding this comment.
Great call. I've done this in the other PR (#4267)
| enable_commenter: bool = False, | ||
| commenter_options: dict = None, | ||
| enable_attribute_commenter: bool = False, | ||
| ): |
There was a problem hiding this comment.
Should we keep the old return type of -> ConnectionT?
| enable_commenter: bool = False, | ||
| commenter_options: dict = None, | ||
| enable_attribute_commenter=None, | ||
| ): |
There was a problem hiding this comment.
Same here, should we keep -> ConnectionT?
There was a problem hiding this comment.
For psycopg2 maybe not as it doesn't support the same Async* types that psycopg(3) does.
| connection._is_instrumented_by_opentelemetry = False | ||
|
|
||
| if not connection._is_instrumented_by_opentelemetry: | ||
| setattr( |
There was a problem hiding this comment.
Won't be doing that PR anymore
| base_factory: type[psycopg.Cursor] | None = None, | ||
| tracer_provider: TracerProvider | None = None, | ||
| db_api: dbapi.DatabaseApiIntegration = None, | ||
| base_factory: psycopg.Cursor = None, |
There was a problem hiding this comment.
I think we should keep as base_factory: type[psycopg.Cursor] | None = None
| def _new_cursor_factory(db_api=None, base_factory=None, tracer_provider=None): | ||
| def _new_cursor_factory( | ||
| db_api: dbapi.DatabaseApiIntegration = None, | ||
| base_factory: pg_cursor = None, |
There was a problem hiding this comment.
I think would be something like base_factory: typing.Optional[typing.Type[pg_cursor]] = None
github-project-automation
bot
moved this from Ready for review
to Done
in Python PR digest
11 participants
Description
Fixes #3070
Depends on #3043 to stop errors with manual tests (instrumenting an app and calling
instrument_connection). Fixes docs and there will be merge conflicts.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
tox -e py312-test-instrumentation-psycopg2tox -e py312-test-instrumentation-psycopgPsycopg2Instrumentor.instrument_connectionPsycopgInstrumentor.instrument_connectionDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.