Skip to content

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

Merged
lzchen merged 1 commit intoopen-telemetry:mainfrom
rjduffner:pymemcache_instruments_version_bump
Apr 6, 2022
Merged

Update pymemcache instrumentation to support pymemcache >=1.3.5,<4#935
lzchen merged 1 commit intoopen-telemetry:mainfrom
rjduffner:pymemcache_instruments_version_bump

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

rjduffner commented Mar 9, 2022

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

rjduffner commented Mar 9, 2022

@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

rjduffner commented Mar 10, 2022

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

rjduffner commented Mar 15, 2022

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
@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
# limitations under the License.


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

@rjduffner rjduffner Mar 23, 2022

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

@lzchen lzchen Apr 4, 2022

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

@rjduffner rjduffner Apr 5, 2022

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