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

Update pymemcache instrumentation to support pymemcache >=1.3.5,<4 #935

Merged
merged 1 commit into from
Apr 6, 2022
Merged

Update pymemcache instrumentation to support pymemcache >=1.3.5,<4 #935

merged 1 commit into from
Apr 6, 2022

Conversation

rjduffner
Copy link
Contributor

@rjduffner rjduffner commented Mar 1, 2022 •
edited
Loading

Description

Update pymemcache instrumentation to support newer versions of pymemcache

Fixes #842
Fixes #907

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Updated tests to run against newer versions of pymemcache.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

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

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@rjduffner rjduffner requested a review from a team March 1, 2022 16:45
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 1, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: rjduffner / name: RJ Duffner (1cb28a6)

@rjduffner rjduffner force-pushed the pymemcache_instruments_version_bump branch from 7f1c560 to 1cb28a6 Compare March 4, 2022 23:21
@ocelotl
Copy link
Contributor

ocelotl commented Mar 9, 2022

@rjduffner please sign the CLA.

@rjduffner
Copy link
Contributor Author

Hi @ocelotl sorry for the delay. I am still working through approvals at my company.

Hopefully will have a signed CLA soon

@rjduffner
Copy link
Contributor Author

@ocelotl Signed. On vacation this week. Will look more next week.

@lzchen
Copy link
Contributor

lzchen commented Mar 10, 2022

@rjduffner
Is this still WIP?

@rjduffner
Copy link
Contributor Author

Hi @lzchen, I am on vacation this week. Will look more next week.

@rjduffner rjduffner force-pushed the pymemcache_instruments_version_bump branch from 48df656 to d04abff Compare March 14, 2022 16:28
@rjduffner rjduffner changed the title [WIP] Update pymemcache instrumentation to support pymemcache version > 1.3.x Update pymemcache instrumentation to support pymemcache version > 1.3.x Mar 14, 2022
@rjduffner
Copy link
Contributor Author

Hi @lzchen and @ocelotl, ready for an initial review. Seems like I need an initial approval sometime recently (I didn't when I first submitted)? Are either of you able to provide this?

@rjduffner rjduffner force-pushed the pymemcache_instruments_version_bump branch from d04abff to b7f060f Compare March 16, 2022 20:53
@srikanthccv srikanthccv mentioned this pull request Mar 17, 2022
Closed
@rjduffner rjduffner force-pushed the pymemcache_instruments_version_bump branch 2 times, most recently from 28ff20f to 703f85c Compare March 21, 2022 17:40
ocelotl
ocelotl reviewed Mar 22, 2022
View reviewed changes
instrumentation/README.md Outdated Show resolved Hide resolved
@rjduffner rjduffner force-pushed the pymemcache_instruments_version_bump branch from dcad88f to 0cd26ca Compare March 23, 2022 21:05
rjduffner
rjduffner commented Mar 23, 2022
View reviewed changes
...telemetry-instrumentation-pymemcache/src/opentelemetry/instrumentation/pymemcache/package.py
@@ -13,4 +13,4 @@
# limitations under the License.


_instruments = ("pymemcache ~= 1.3",)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After testing, there was a change in 1.3.5 that added version to the client. Before this the instrumentation fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

So for versions < 1.3.5, the instrumentation did not work properly? According to this, a VERSION command was added. Was curious as to why without this it would cause the instrumentation to break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so the instrumentation uses .version() in the tests. This leads to a test failing. While I am fairly confident it would work for versions < 1.3.5, the tests don't and I wanted to keep this PRs scope low given all of the issues I am already currently having.

So locking the version to what actually passes the test felt like a good start.

@rjduffner rjduffner changed the title Update pymemcache instrumentation to support pymemcache version > 1.3.x Update pymemcache instrumentation to support pymemcache version >= 1.3.5 Mar 23, 2022
@rjduffner rjduffner changed the title Update pymemcache instrumentation to support pymemcache version >= 1.3.5 Update pymemcache instrumentation to support pymemcache >=1.3.5,<4 Mar 23, 2022
@rjduffner rjduffner requested a review from ocelotl March 24, 2022 23:21
@rjduffner rjduffner force-pushed the pymemcache_instruments_version_bump branch from 5f3bb01 to 763709f Compare March 25, 2022 23:15
@rjduffner rjduffner force-pushed the pymemcache_instruments_version_bump branch from 03aab6f to 0c2c470 Compare April 5, 2022 16:22
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 5, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: rjduffner / name: RJ Duffner (2191db5)

@rjduffner rjduffner force-pushed the pymemcache_instruments_version_bump branch from f4fba8f to 2191db5 Compare April 5, 2022 20:14
@lzchen lzchen merged commit b8cad79 into open-telemetry:main Apr 6, 2022
@rjduffner rjduffner deleted the pymemcache_instruments_version_bump branch April 6, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@ocelotl ocelotl Awaiting requested review from ocelotl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for pymemcache 2.x and 3.x Pymemcache instrumenter version selector is for very old package version
3 participants
@rjduffner @ocelotl @lzchen