Skip to content
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

Add docstring for instrument_connection() and tests for database drivers #3108

Merged

Conversation

beijiez
Copy link
Contributor

@beijiez beijiez commented Dec 13, 2024

Description

Add docstring for instrument_connection() and tests for database drivers as it is missing. This will be useful to new database instrumentation efforts

DB driver docstrings updated:

[x] aiopg
[x] mysql-connector
[x] mysqlclient
[x] psycopg
[x] psycopg2
[x] pymysql
[x] sqlite3

Addresses #3089

How Has This Been Tested?

  • Added tests for DB driver /tests if it was missing
  • Tested Usage code locally
  • tox -e spellcheck OK
  • tox congratulations :)

Does This PR Require a Core Repo Change?

No

Copy link

linux-foundation-easycla bot commented Dec 13, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@beijiez beijiez marked this pull request as ready for review December 13, 2024 21:17
@beijiez beijiez changed the title [WIP] Add docstring for instrument_connection() and tests for database drivers Add docstring for instrument_connection() and tests for database drivers Dec 13, 2024
@xrmx xrmx added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 17, 2024
@lzchen lzchen requested review from tammy-baylis-swi and removed request for federicobond December 17, 2024 16:38
@tammy-baylis-swi
Copy link
Contributor

Thanks @beijiez ! I've requested one set of changes first.

There are a few CI formatting checks failing. Here are some more individual local commands if you could please run on your local:

  1. tox -e ruff
  2. tox -e lint-instrumentation-psycopg2
  3. tox -e lint-instrumentation-sqlite3
  4. tox -e docs <-- this might only work for Python 3.11 or 3.10

@beijiez
Copy link
Contributor Author

beijiez commented Dec 17, 2024

Thanks @beijiez ! I've requested one set of changes first.

There are a few CI formatting checks failing. Here are some more individual local commands if you could please run on your local:

  1. tox -e ruff
  2. tox -e lint-instrumentation-psycopg2
  3. tox -e lint-instrumentation-sqlite3
  4. tox -e docs <-- this might only work for Python 3.11 or 3.10

Thank you for the fast review @tammy-baylis-swi! Updated and re-ran CI locally

tox -e ruff
tox -e lint-instrumentation-psycopg2
tox -e lint-instrumentation-sqlite3
tox -e lint-instrumentation-aiopg
tox -e docs [on a 3.11.0 pyvenv] 

@tammy-baylis-swi
Copy link
Contributor

Thank you again @beijiez ! Nice to see all CI passing now. I've left more suggestions and a question when you have a moment.

@beijiez
Copy link
Contributor Author

beijiez commented Dec 19, 2024

Thank you again @beijiez ! Nice to see all CI passing now. I've left more suggestions and a question when you have a moment.

Thank you for the detailed review @tammy-baylis-swi ! I responded to your question, re-verified CI, and pushed changes

Copy link
Contributor

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the examples from the code-blocks don't run as they are written. Can we add the table creation before the insertion?

It saves time for those reading the docs when trying the code snippets.

@tammy-baylis-swi
Copy link
Contributor

All the examples from the code-blocks don't run as they are written. Can we add the table creation before the insertion?

@Kludex could you provide a suggestion on one of the instrumentor scripts for how to do this?

@beijiez
Copy link
Contributor Author

beijiez commented Dec 19, 2024

All the examples from the code-blocks don't run as they are written. Can we add the table creation before the insertion?

It saves time for those reading the docs when trying the code snippets.

Good callout, sqlite3 currently has a working example with table creation. Can add it to rest of the docstrings

cursor.execute("CREATE TABLE test (testField INTEGER)")

@tammy-baylis-swi
Copy link
Contributor

All the examples from the code-blocks don't run as they are written. Can we add the table creation before the insertion?
It saves time for those reading the docs when trying the code snippets.

Good callout, sqlite3 currently has a working example with table creation. Can add it to rest of the docstrings

cursor.execute("CREATE TABLE test (testField INTEGER)")

Ah, I see. I was thinking docs/formatting tables instead of db tables. 😅

@Kludex
Copy link
Contributor

Kludex commented Dec 19, 2024

Yeah, I updated the sqlite3 one last week. 😁

That would be great. Thanks!

@beijiez
Copy link
Contributor Author

beijiez commented Dec 19, 2024

Yeah, I updated the sqlite3 one last week. 😁

That would be great. Thanks!

Done. Added cursor.execute("CREATE TABLE IF NOT EXISTS test (testField INTEGER)") to each to allow idempotent table creation in the example docstrings

Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, thank you for all the work!

This will be checked by the Maintainers next.

@lzchen lzchen merged commit 1092344 into open-telemetry:main Dec 20, 2024
573 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants