Skip to content

Conversation

@nickamorim
Copy link
Contributor

Description

Dalli supports Memcached's meta protocol as of 3.2.0. This PR adds support for instrumentation when clients are using the meta protocol.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 14, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@nickamorim nickamorim force-pushed the nickamorim/dalli-meta-support branch from 112ec4b to 4fe2450 Compare April 14, 2025 18:38
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Hi @nickamorim! Thanks for your contribution!

There's a comment in the server patch that should be updated too:

# Module to prepend to Dalli::Server (or Dalli::Protocol::Binary in 3.0+) for instrumentation

I wonder if we should rename the patch now that it applies to so many different classes?

::Dalli::Server.prepend(Patches::Server)
else
::Dalli::Protocol::Binary.prepend(Patches::Server)
::Dalli::Protocol::Meta.prepend(Patches::Server) if defined?(::Dalli::Protocol::Meta)
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests currently run on the default protocol.

In the latest version, this should default to binary. Now that the protocol could be :binary or :meta, what do you think about updating the tests to run on a :meta protocol?

@nickamorim
Copy link
Contributor Author

nickamorim commented Apr 14, 2025

I wonder if we should rename the patch now that it applies to so many different classes?

I'm not exactly sure what this would look like but it seems that Dalli::Server is deprecated in favour of specifying the protocol https://github.com/petergoldstein/dalli/blob/main/lib/dalli/server.rb#L4.

As an aside, did you want me to update the version too as part of this PR? Nvm, I read https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/CONTRIBUTING.md#releases

@nickamorim nickamorim force-pushed the nickamorim/dalli-meta-support branch from 1051959 to b12e3f8 Compare April 14, 2025 23:51
@robertlaurin robertlaurin merged commit db1286a into open-telemetry:main Apr 15, 2025
60 checks passed
yiyuan-he pushed a commit to yiyuan-he/opentelemetry-ruby-contrib that referenced this pull request Apr 30, 2025
* feat: support meta protocol instrumentation

* test: dalli meta and binary protocol instrumentation support

---------

Co-authored-by: Robert <[email protected]>
yiyuan-he pushed a commit to yiyuan-he/opentelemetry-ruby-contrib that referenced this pull request May 14, 2025
* feat: support meta protocol instrumentation

* test: dalli meta and binary protocol instrumentation support

---------

Co-authored-by: Robert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants