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

Renaming of multi? to quiet? can be breaking #865

Closed
larouxn opened this issue Dec 13, 2021 · 11 comments
Closed

Renaming of multi? to quiet? can be breaking #865

larouxn opened this issue Dec 13, 2021 · 11 comments

Comments

@larouxn
Copy link

larouxn commented Dec 13, 2021

👋 Just a heads up issue that the recent 3.1.1 patch version release contained a rename of multi? to quiet? which was a breaking change (no method error) for a service I was running. Not sure if that will be the case for many apps/services but just wanted to give a heads up. Maybe we can add a note to the History.md warning people of potential breakage.

@petergoldstein
Copy link
Owner

Let me check on that. I thought I'd aliased all the public methods

@petergoldstein
Copy link
Owner

@larouxn So multi? wasn't a method on Dalli::Client. How is your service utilizing this method? Are you directly accessing the Dalli::Protocol::Binary?

@larouxn
Copy link
Author

larouxn commented Dec 13, 2021

@petergoldstein, the calls are indeed directly on Dalli::Protocol::Binary. Still trying to track down exactly what is making the multi? calls to see if it can either stop making calls on Binary or address the outdated method name.

NoMethodError · undefined method `multi?' for #<Dalli::Protocol::Binary:0x00007fd81ed3e4d0>

If it's a public gem making the multi? calls, I hope we can address the issue. Will report back here if I find anything.

@larouxn
Copy link
Author

larouxn commented Dec 13, 2021

👋 Alright, I was able to find the multi? method usage. Thankfully just an internal library, not in a public gem so this may be a false alarm re: breaking change. Granted, other people outside my organization may also be using that method directly, in which case would still be breaking for them. Maybe still worth noting.

@petergoldstein
Copy link
Owner

@larouxn Ok, the multi? method on Dalli::Protocol::Binary was private. It's definitely not something that users of the library should be calling in general. Can you share why your internal library was calling it?

@larouxn
Copy link
Author

larouxn commented Dec 13, 2021

It's used in an instrumentation library that I'm looking at for the first time right now. Seems to be used to ensure we don't instrument Dalli call latency during multi calls such as multi_get as that would be statistically inaccurate compare to non-multi methods calls. Seems normal multi is also used for "quiet operations".

Hoping to tag someone with more context into here.

@robertlaurin
Copy link

Hey @petergoldstein we're doing something similar for dalli auto instrumentation in the opentelemetry-ruby repo.

Ideally we (Otel ruby folks) would love to push for first party instrumentation in the dalli gem itself (let me know if you're open to this idea, I would be happy to contribute PRs towards this).

However I don't want to try and pull this issue onto a tangent, would you be open to supplying a public method for performing this check so we could stop doing unsavoury things like relying on a private method?

@petergoldstein
Copy link
Owner

@robertlaurin So addressing this in order:

  1. Yes, I'd be open to including first party instrumentation, provided the output is suitably generic and not tied to particular implementations. If you could put an answer here, that would be a great start. And PRs would be welcome.
  2. I am open to moving the quiet? method to public, and maintaining that for the 3.x version. I can even add an alias for multi?.

One note about all of this is that the quiet/multi block is not the only place where we use quiet memcached commands (which is a little confusing). Pipelined gets use quiet memcached commands without setting the thread context variable used by quiet?, so that may impact your instrumentation.

@petergoldstein petergoldstein changed the title Renaming of mutli? to quiet? can be breaking Renaming of multi? to quiet? can be breaking Dec 13, 2021
@petergoldstein
Copy link
Owner

@robertlaurin and @larouxn Take a look at this - #866 . Let me know if that simplifies things for you. I'm ok packaging this and releasing as a 3.1.2 today.

@robertlaurin
Copy link

If you could put an answer here, that would be a great start. And PRs would be welcome.

Will do!

I am open to moving the quiet? method to public, and maintaining that for the 3.x version. I can even add an alias for multi?.

That is very helpful, thank you!

One note about all of this is that the quiet/multi block is not the only place where we use quiet memcached commands (which is a little confusing). Pipelined gets use quiet memcached commands without setting the thread context variable used by quiet?, so that may impact your instrumentation.

I'll have to take a closer look at various places I know this being used to see what impact it could have on the accuracy on our instrumentation. Appreciate you flagging that.

@petergoldstein
Copy link
Owner

@robertlaurin @larouxn I've merged #866 and released 3.1.2 with that included. Hopefully that addresses the issue here. Please let me know if it doesn't.

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

No branches or pull requests

3 participants