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.

Sign up for GitHub

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

Jump to bottom

Add pymssql instrumentation #394

Merged
merged 3 commits into from
Jan 27, 2025
Merged

Add pymssql instrumentation #394

merged 3 commits into from
Jan 27, 2025

Conversation

guillaumep
Copy link
Contributor

@guillaumep guillaumep commented Apr 1, 2021 •
edited
Loading

Description

This pull request implements instrumentation for the pymssql library (https://pypi.org/project/pymssql/).
It is basically a copy of the pymysql instrumentation.

Still a work-in-progress.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • Ran tox tests
  • Tested manually in a python program - verified that traces are properly sent

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated [PARTIAL] -> I can complete that once the PR is opened and I have an issue number
  • Unit tests have been added
  • Documentation has been updated

@guillaumep guillaumep requested review from a team, owais and lzchen and removed request for a team April 1, 2021 19:03
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 1, 2021 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@guillaumep guillaumep marked this pull request as draft April 1, 2021 19:28
@guillaumep
Copy link
Contributor Author

It seems from the CI logs that pymssql does not build for pypy3.

Would it be acceptable to disable pypy3 support in the tests for this instrumentation?

@guillaumep
Copy link
Contributor Author

guillaumep commented Apr 1, 2021 •
edited
Loading

Also, the docs fails to build because of this error:

sphinx.errors.SphinxWarning: autodoc: failed to import module 'pymssql' from module 'opentelemetry.instrumentation'; the following exception was raised:
No module named 'pymssql'

I'm trying to figure out my error, can someone experienced with the repo help?

@guillaumep guillaumep force-pushed the instrument_pymssql branch from 0860e38 to 92d7340 Compare April 7, 2021 04:03
@guillaumep
Copy link
Contributor Author

I had to subclass DatabaseApiIntegration to obtain pymssql's connection attributes, as the attributes are not kept by the library in the Connection object. You need to get them from the connect function parameter.

@guillaumep guillaumep force-pushed the instrument_pymssql branch from 92d7340 to faeea0a Compare July 22, 2021 17:48
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 22, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@guillaumep guillaumep force-pushed the instrument_pymssql branch 3 times, most recently from 0e2df03 to b93ad00 Compare July 22, 2021 18:57
@lzchen
Copy link
Contributor

lzchen commented Feb 2, 2022

@guillaumep
Are you still working on this?

@guillaumep
Copy link
Contributor Author

guillaumep commented Feb 2, 2022 •
edited
Loading

@lzchen I was expecting help with the issues I mentioned in the comments above, but since I never got any replies up to now I had put this work aside.

pymssql instrumention is still a feature we want at work (outbox.com), so if I can have proper support regarding my questions I'll be glad to finish this PR.

Let me rebase the PR this week and post back here with the current issues I will have.

@lzchen
Copy link
Contributor

lzchen commented Feb 3, 2022 •
edited
Loading

@guillaumep
Apologies for that. Usually PRs are not reviewed unless if marked as review (changed to an actual PR). If you would like help with your build issues, feel free to rebase and we can get you the assistance you need.

@guillaumep guillaumep force-pushed the instrument_pymssql branch 2 times, most recently from 17d054f to 0333424 Compare August 26, 2022 15:03
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 26, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@guillaumep guillaumep force-pushed the instrument_pymssql branch 4 times, most recently from 96980a5 to 4cc4753 Compare August 26, 2022 22:06
guillaumep
guillaumep commented Aug 26, 2022
View reviewed changes
tox.ini Outdated Show resolved Hide resolved
@guillaumep guillaumep marked this pull request as ready for review August 26, 2022 22:55
@guillaumep guillaumep changed the title [WIP] Add pymssql instrumentation Add pymssql instrumentation Aug 26, 2022
@guillaumep
Copy link
Contributor Author

@lzchen now ready for review!

emdneto
emdneto reviewed Jan 23, 2025
View reviewed changes
tox.ini Outdated Show resolved Hide resolved
@lzchen
Copy link
Contributor

lzchen commented Jan 23, 2025

@guillaumep Thanks for the contribution. Would you be willing to add yourself as an owner for this component under here?

Thanks for the patience on this review. Would you be able to add your name to component-owners.yml?

@guillaumep
Copy link
Contributor Author

Thanks for the patience on this review. Would you be able to add your name to component-owners.yml?

For now yes.

guillaumep

This comment was marked as off-topic.

Sign in to view

@guillaumep guillaumep force-pushed the instrument_pymssql branch 2 times, most recently from 37bcf17 to 01f7b13 Compare January 23, 2025 21:28
@guillaumep
Copy link
Contributor Author

@emdneto @lzchen I think I have resolved all items, but let me know.

lzchen
lzchen approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Couple of unresolved comments but other than that LGTM

xrmx
xrmx approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

With the versioning fixed and the bootstrap_gen updated LGTM

instrumentation/opentelemetry-instrumentation-pymssql/pyproject.toml Outdated Show resolved Hide resolved
@guillaumep guillaumep force-pushed the instrument_pymssql branch 3 times, most recently from a6c7885 to 6d4377a Compare January 24, 2025 19:53
@xrmx
Copy link
Contributor

xrmx commented Jan 27, 2025

@guillaumep please run tox -e generate and tox -e generate-workflows . Also no need to squash everything, we squash on merge

emdneto
emdneto approved these changes Jan 27, 2025
View reviewed changes
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

LGTM when CI gets fixed

@guillaumep
Copy link
Contributor Author

@xrmx @emdneto Rebased on main to solve conflict and ran tox as requested.

@xrmx xrmx enabled auto-merge (squash) January 27, 2025 14:46
@xrmx xrmx disabled auto-merge January 27, 2025 14:46
@xrmx xrmx enabled auto-merge (squash) January 27, 2025 14:51
@xrmx xrmx merged commit 5478a0b into open-telemetry:main Jan 27, 2025
694 checks passed
@ChristianWeyer
Copy link

What would be your recommendation to use the package as long as it has not been published yet @guillaumep ?
Needs to work locally as well as in Docker builds.

@guillaumep
Copy link
Contributor Author

What would be your recommendation to use the package as long as it has not been published yet @guillaumep ? Needs to work locally as well as in Docker builds.

There should be a release shortly, in one or two weeks. In the meantime you can use the same technique as I suggested here to produce a package you may manuall install using pip: #394 (comment)

Installing a local package could be done for a Docker build, copying the file to the Docker image then doing a pip install for example. Please ask a question on Stack Overflow or similar forum if you need help with that.

aryabharat pushed a commit to aryabharat/opentelemetry-python-contrib that referenced this pull request Feb 2, 2025
@guillaumep @xrmx @aryabharat
Add pymssql instrumentation (open-telemetry#394)
9fc80e9 
* Add pymssql instrumentation

* Run tox -e generate

* tox -e generate-workflows

---------

Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nemoshlag nemoshlag nemoshlag left review comments

@xrmx xrmx xrmx approved these changes

@emdneto emdneto emdneto approved these changes

@lzchen lzchen lzchen approved these changes

@tammy-baylis-swi tammy-baylis-swi tammy-baylis-swi approved these changes

@owais owais Awaiting requested review from owais

Assignees

@guillaumep guillaumep

@nemoshlag nemoshlag

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@guillaumep @lzchen @srikanthccv @ChristianWeyer @tammy-baylis-swi @xrmx @nemoshlag @emdneto