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

Memcached Bytes Replaced with Version #759

Closed
dinobot71 opened this issue Sep 14, 2020 · 5 comments · Fixed by #760
Closed

Memcached Bytes Replaced with Version #759

dinobot71 opened this issue Sep 14, 2020 · 5 comments · Fixed by #760

Comments

@dinobot71
Copy link

I was all set to use this package as our memcached adaptor, everything looked good. But when we actually tried to use it, its not usable.

phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php

line: 73:

$stats['bytes'] = (isset($stats['bytes']) ? $stats['version'] : 0);

is magically replacing bytes with the version, so its completely impossible to track the percentage of cache usage.

Why is bytes being overridden with the version? Not only does it break statistics coming from memcache, but there are no comment in the code about the justification for breaking the standard behavior of memcache, and no alternative route is given to getting the stats as the are supposed to be.

@github-actions
Copy link

Hello curious contributor !
Since it seems to be your first contribution, make sure that you've been:

  • Reading and searching out our WIKI
  • Reading and agreed with our Code Of Conduct
  • Reading and understood our Coding Guideline
  • Reading our README
    If everything looks unclear to you, tell us what 😄
    The Phpfastcache Team

@Geolim4
Copy link
Member

Geolim4 commented Sep 15, 2020

Hello,

Please reformat the issue using the default's issue template as I'm not a magician 👍🏼

Thanks you,
Georges

@dinobot71
Copy link
Author

dinobot71 commented Sep 15, 2020

Configuration

  • PhpFastCache version: 8.0.2
  • PhpFastCache API version: 3.0.0
  • PHP version: 7.4.7

Describe the bug

Per above, it appears the original coder copy and pasted the line for 'bytes' in Memcached driver, but forget to change 'version' to 'bytes'. So when you ask for the stats:

$this->memcached = CacheManager::getInstance('memcached', new Config([ 'host' => 'memcached', 'port' => 11211 ])); $bytesUsed = $this->memcached->getStats()->getRawData()['bytes'];
you get the version, not the bytes. This in turn breaks monitoring of memcached, since you can't calculate things like available cache percentage.

To Reproduce

Will reproduce no matter way you ask for the stats, because its always using the wrong value for bytes.

Expected behavior
When you ask for 'bytes' you should get 'bytes' not 'version''.

Additional context
Architecturally; I don't actually want bytes, I want summary statistics I can do monitoring with. If you are inclined to add a better feature here that can be supported across drivers, I would happily used that instead. Key metrics for monitoring:

return (object)[ 'uptime' => $data['uptime'], 'rbytes' => $data['bytes_read'], 'wbytes' => $data['bytes_written'], 'hitratio' => trim(sprintf("%6.2f%%", ($data['get_hits'] / $data['cmd_get']) * 100)), 'evictions' => $data['evictions'], 'used' => $data['bytes'], 'capacity' => $data['limit_maxbytes'], 'full' => trim(sprintf("%6.2f%%", $data['bytes'] / $data['limit_maxbytes'])), 'set' => $data['cmd_get'], 'get' => $data['cmd_set'] ];
Similar to:

https://blog.serverdensity.com/monitor-memcached/
https://docs.datadoghq.com/integrations/mcache/?tab=host

I've already edited my copy to make fix the clerical error, (presuming it is in fact just a clerical error). But as I say, if you think I should instead be using something more independent of the driver, I don't disagree, but I need to know how to get the monitoring in whatever the proper way is.

Geolim4 added a commit that referenced this issue Sep 15, 2020
@Geolim4 Geolim4 self-assigned this Sep 15, 2020
Geolim4 added a commit to Geolim4/phpfastcache that referenced this issue Sep 15, 2020
@Geolim4 Geolim4 added the 7.1 label Sep 15, 2020
@Geolim4 Geolim4 reopened this Sep 15, 2020
@Geolim4
Copy link
Member

Geolim4 commented Sep 15, 2020

Hello,

I identified the issue on v8 and v7.

The commits are waiting for build to pass before merging.

Then I'll make a new release by the end of week.

Cheers,
Georges

@dinobot71
Copy link
Author

most excellent, thank you sir.

@Geolim4 Geolim4 changed the title Memcached Bytes Replaced wiith Version? Memcached Bytes Replaced with Version? Nov 23, 2020
@Geolim4 Geolim4 changed the title Memcached Bytes Replaced with Version? Memcached Bytes Replaced with Version Nov 23, 2020
@Geolim4 Geolim4 linked a pull request Nov 8, 2021 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants