feat(redis): Add mget/mset support #156

b-viguier · 2021-04-29T19:55:38Z

Add support for Redis::mget and Redis::mset functions, as suggested in #130

To be honest, I copied/pasted some code from existing Redis::get and Redis::set support 😅

CLAassistant · 2021-04-29T19:55:50Z

All committers have signed the CLA.

nr-opensource-bot · 2021-04-29T19:56:00Z

Can one of the admins verify this patch?

zsistla

Hello there and thanks so much for engaging on this issue. It's great to see the test case updated as well, thanks!!

b-viguier · 2021-04-30T06:53:56Z

Thanks @zsistla 😄
I also noticed another place to add a test in test_instance_basic.php, to mimic existing tests for set/get. Do you think it's enough?
Is it possible to approve running workflows for this PR? I did my best to run them locally, but I would be more confident if it runs elsewhere too 😅

I know that even if this PR is accepted, it may take a long time for NR team to release a new version of the extension. So, do you know if it's safe to run my own agent build in production? Of course, I cannot take the responsibility of my changes, but at least is it technically feasible? Does the extension require some "internal signature" preventing to run invalid agent build in production? I would be interested to be able to run some private beta version, waiting for next official versions in this case.

By the way, if this PR needs some rework, feel free to give me some hints about how to proceed 👍
Have a nice day!

zsistla · 2021-04-30T14:04:01Z

@b-viguier , we always welcome more test cases/updates to ensure proper functionality :)

One think I just noticed was that we have recently moved to the convention of opening all PRs against the dev branch. I opened a PR to update the documentation, but would you be able to open this PR against dev?

Additionally, to your question about internal signature/invalid build: there is none. It's totally open source, so if you build, for instance, off of the 9.17.1 branch, it is complete and should give you all the functionality of the latest release. :)

regards and thanks again!

b-viguier · 2021-04-30T14:13:35Z

One think I just noticed was that we have recently moved to the convention of opening all PRs against the dev branch. I opened a PR to update the documentation, but would you be able to open this PR against dev?

Done ✅

Additionally, to your question about internal signature/invalid build: there is none. It's totally open source, so if you build, for instance, off of the 9.17.1 branch, it is complete and should give you all the functionality of the latest release. :)

Good news! I will give it a try 👍

tests/integration/redis/test_instance_basic.php

zsistla · 2021-05-03T18:36:23Z

ok jenkins

zsistla · 2021-05-03T19:49:25Z

tests/integration/redis/test_basic.php

@@ -83,6 +86,10 @@ function test_basic() {
  tap_equal(1, $redis->del($key), 'delete key');
  tap_equal(0, $redis->del($key), 'delete missing key');

+  tap_assert($redis->mset([$key => 'bar']), 'mset key');
+  tap_equal(['bar'], $redis->mget($key), 'mget key');


Hi @b-viguier ,

Looks like the tests caught an error in the testcase:

Warning: Redis::mget() expects parameter 1 to be array, string given

mget is expecting an array. I think if you put $key in brackets [$key] it should work.

Thanks @zsistla, you're totally right! It's fixed now 👍

I'm really annoyed, because I did my best to run tests locally and I didn't notice anything wrong 😕 … I'm using the Docker image stored in Github actions: https://github.com/newrelic/newrelic-php-agent/blob/main/.github/actions/ubuntu20-build-action/Dockerfile
The output is very dense, did I miss something? Furthermore, I didn't find where the CI reported the error, are you using tools internal to NR?

Thanks @b-viguier,

No worries. There are two sets of tests, the unit tests and our integration tests.

The output from the unit tests can be seen in the github CI output, but we're still in the process of moving from our old CI to github actions for the CI, so the integration tests are are still run internally.

zsistla · 2021-05-04T14:17:09Z

ok jenkins

zsistla · 2021-05-04T16:50:39Z

Hi @b-viguier ,
Thanks so much for your quick turnarounds on this; it is almost there, but needs a few more updates to the test cases to pass.

in test_basic.php , you'll notice an expect block. This block is what we are expecting to be reported by the agent.

Currently, it's expecting 8 redis calls, but you added three more so we need to update that number to 11. Also, delete was called one more time, so we increment the number to 4 and add mget/mset sections because now we can se mget/mset thanks to your instrumentation :)

Something like this should do it:

/*EXPECT_METRICS
[
  "?? agent run id",
  "?? start time",
  "?? stop time",
  [
    [{"name":"Datastore/all"},                         [11, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/allOther"},                    [11, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/Redis/all"},                   [11, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/Redis/allOther"},              [11, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/connect"},     [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/connect",
      "scope":"OtherTransaction/php__FILE__"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/del"},         [4, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/del",
      "scope":"OtherTransaction/php__FILE__"},         [4, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/get"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/get",
      "scope":"OtherTransaction/php__FILE__"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/set"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/set",
      "scope":"OtherTransaction/php__FILE__"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/mget"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/mget",
      "scope":"OtherTransaction/php__FILE__"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/mset"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/mset",
      "scope":"OtherTransaction/php__FILE__"},         [1, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/setnx"},       [2, "??", "??", "??", "??", "??"]],
    [{"name":"Datastore/operation/Redis/setnx",
      "scope":"OtherTransaction/php__FILE__"},         [2, "??", "??", "??", "??", "??"]],
    [{"name":"OtherTransaction/all"},                  [1, "??", "??", "??", "??", "??"]],
    [{"name":"OtherTransaction/php__FILE__"},          [1, "??", "??", "??", "??", "??"]],
    [{"name":"OtherTransactionTotalTime"},             [1, "??", "??", "??", "??", "??"]],
    [{"name":"OtherTransactionTotalTime/php__FILE__"}, [1, "??", "??", "??", "??", "??"]]
  ]
]
*/

Also, in test_instance_basic.php, mget/mset should be added to the matches here.

b-viguier · 2021-05-06T13:49:26Z

Thanks for you help @zsistla , I think I got it 👍
Let me know if something still rings in your CI 😉

zsistla · 2021-05-06T13:58:50Z

ok jenkins

zsistla · 2021-05-06T23:57:48Z

ok jenkins

zsistla · 2021-05-07T00:55:25Z

:) Thanks, @b-viguier, it's looking great with one caveat....

While we don't support 5.3 anymore, we are still testing against it, and that PHP version showed some incompatibility with the mget/mset format.

There's a SKIPIF block in both your test files.
If you put the following, it should be good to pass.

/*SKIPIF
<?php
if (version_compare(phpversion(), '5.4', '<')) {
    die("skip: PHP > 5.3 required\n");
}
require("skipif.inc");
*/

zsistla · 2021-05-12T13:04:47Z

ok jenkins

zsistla · 2021-05-12T14:05:00Z

Hi @b-viguier , it's looking great.
tests/integration/redis/test_instance_basic.php passed all configurations with the new skipif block!
tests/integration/redis/test_basic.php needs the same skipif updated as well, then it should be ready to merge :)

b-viguier · 2021-05-12T14:50:36Z

Sorry, I forgot the second file 😕
Looking forward for #162 to be able to run tests locally and identify these mistakes by myself 😅

zsistla · 2021-05-12T14:55:26Z

ok jenkins

@stockalexander

--- subject: PHP agent releaseDate: '2021-08-23' version: 9.18.1.303 downloadLink: 'https://download.newrelic.com/php_agent/archive/9.18.1.303' --- ## New Relic PHP Agent v9.18.1 ## ### New Features ### * [Added](newrelic/newrelic-php-agent#162) a docker development environment. It's now possible for contributors to both develop and test (unit tests and integration tests) without setting up a specific environment on their own system. Please see our [documentation](https://github.com/newrelic/newrelic-php-agent/blob/main/docs/dev_environment.md) for more information. * [Route caching in `Laravel 7.x` is now supported!](newrelic/newrelic-php-agent#174). Transaction naming now works with routes cached via `php artisan route:cache`. @stockalexander, thanks for your contribution! * `Redis::mget` and `Redis::mset` [functions are now supported](newrelic/newrelic-php-agent#156). @b-viguier, thanks for your contribution! ### Bug Fixes ### * [Fixed](newrelic/newrelic-php-agent#161) instances where a memory leak was occurring with our `curl multi` instrumentation. * [Fixed](newrelic/newrelic-php-agent#176) an issue where a supportability metric used to track an edge case was causing a segfault. * [Fixed](newrelic/newrelic-php-agent#87) an issue where PHP versions with an unknown API version were incorrectly handled during Debian package install. * [Fixed](newrelic/newrelic-php-agent#164) instances where `parent.transportDuration` values are `0` for transactions between two PHP applications instrumented through distributed tracing. @b-viguier, thanks for your contribution! * [Fixed](newrelic/newrelic-php-agent#158) an issue where the `newrelic.ini` configuration file was incorrectly installed. @b-viguier, thanks for your contribution! ### Support Statement ### * New Relic recommends that you upgrade the agent regularly and at a minimum every 3 months. As of this release, the oldest supported version is [8.6.0](/docs/release-notes/agent-release-notes/php-release-notes/php-agent-860238/).

@stockalexander

--- subject: PHP agent releaseDate: '2021-08-23' version: 9.18.1.303 downloadLink: 'https://download.newrelic.com/php_agent/archive/9.18.1.303' --- ## New Relic PHP Agent v9.18.1 ## ### New Features ### * [Added](newrelic/newrelic-php-agent#162) a docker development environment. It's now possible for contributors to both develop and test (unit tests and integration tests) without setting up a specific environment on their own system. Please see our [documentation](https://github.com/newrelic/newrelic-php-agent/blob/main/docs/dev_environment.md) for more information. * [Route caching in `Laravel 7.x` is now supported!](newrelic/newrelic-php-agent#174). Transaction naming now works with routes cached via `php artisan route:cache`. @stockalexander, thanks for your contribution! * `Redis::mget` and `Redis::mset` [functions are now supported](newrelic/newrelic-php-agent#156). @b-viguier, thanks for your contribution! ### Bug Fixes ### * [Fixed](newrelic/newrelic-php-agent#161) instances where a memory leak was occurring with our `curl multi` instrumentation. * [Fixed](newrelic/newrelic-php-agent#176) an issue where a supportability metric used to track an edge case was causing a segfault. * [Fixed](newrelic/newrelic-php-agent#87) an issue where PHP versions with an unknown API version were incorrectly handled during Debian package install. * [Fixed](newrelic/newrelic-php-agent#164) instances where `parent.transportDuration` values are `0` for transactions between two PHP applications instrumented through distributed tracing. @b-viguier, thanks for your contribution! * [Fixed](newrelic/newrelic-php-agent#158) an issue where the `newrelic.ini` configuration file was incorrectly installed. @b-viguier, thanks for your contribution! ### Support Statement ### * New Relic recommends that you upgrade the agent regularly and at a minimum every 3 months. As of this release, the oldest supported version is [8.6.0](/docs/release-notes/agent-release-notes/php-release-notes/php-agent-860238/).

zsistla previously approved these changes Apr 30, 2021

View reviewed changes

b-viguier dismissed zsistla’s stale review via 4ddcf01 April 30, 2021 06:46

b-viguier force-pushed the feat/redis-functions branch from 0459472 to 4ddcf01 Compare April 30, 2021 06:46

b-viguier marked this pull request as ready for review April 30, 2021 07:02

b-viguier changed the base branch from main to dev April 30, 2021 14:06

b-viguier force-pushed the feat/redis-functions branch from 4ddcf01 to f08d8fc Compare April 30, 2021 14:12

zsistla requested review from kneitinger, Fahmy-Mohammed and joshuabenuck April 30, 2021 16:59

b-viguier force-pushed the feat/redis-functions branch from f08d8fc to 49b56ad Compare May 2, 2021 08:00

b-viguier commented May 2, 2021

View reviewed changes

tests/integration/redis/test_instance_basic.php Show resolved Hide resolved

joshuabenuck approved these changes May 3, 2021

View reviewed changes

zsistla reviewed May 3, 2021

View reviewed changes

b-viguier force-pushed the feat/redis-functions branch from 49b56ad to b5d9867 Compare May 4, 2021 08:45

b-viguier force-pushed the feat/redis-functions branch from b5d9867 to c57acff Compare May 6, 2021 13:48

b-viguier force-pushed the feat/redis-functions branch from c57acff to 3af8e0d Compare May 11, 2021 06:36

feat(redis): Add mget/mset support

990a9c8

b-viguier force-pushed the feat/redis-functions branch from 3af8e0d to 990a9c8 Compare May 12, 2021 14:48

zsistla approved these changes May 12, 2021

View reviewed changes

zsistla merged commit 8787230 into newrelic:dev May 12, 2021

b-viguier deleted the feat/redis-functions branch May 17, 2021 15:53

Fahmy-Mohammed pushed a commit that referenced this pull request Aug 3, 2021

feat(redis): Add mget/mset support (#156)

9994350

zsistla mentioned this pull request Aug 23, 2021

Create php-agent-9181303.mdx newrelic/docs-website#3601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(redis): Add mget/mset support #156

feat(redis): Add mget/mset support #156

b-viguier commented Apr 29, 2021

CLAassistant commented Apr 29, 2021 •

edited

Loading

nr-opensource-bot commented Apr 29, 2021

zsistla left a comment

b-viguier commented Apr 30, 2021

zsistla commented Apr 30, 2021

b-viguier commented Apr 30, 2021

zsistla commented May 3, 2021

zsistla May 3, 2021

b-viguier May 4, 2021

zsistla May 4, 2021

zsistla commented May 4, 2021

zsistla commented May 4, 2021

b-viguier commented May 6, 2021

zsistla commented May 6, 2021

zsistla commented May 6, 2021

zsistla commented May 7, 2021

zsistla commented May 12, 2021

zsistla commented May 12, 2021

b-viguier commented May 12, 2021

zsistla commented May 12, 2021

feat(redis): Add mget/mset support #156

feat(redis): Add mget/mset support #156

Conversation

b-viguier commented Apr 29, 2021

CLAassistant commented Apr 29, 2021 • edited Loading

nr-opensource-bot commented Apr 29, 2021

zsistla left a comment

Choose a reason for hiding this comment

b-viguier commented Apr 30, 2021

zsistla commented Apr 30, 2021

b-viguier commented Apr 30, 2021

zsistla commented May 3, 2021

zsistla May 3, 2021

Choose a reason for hiding this comment

b-viguier May 4, 2021

Choose a reason for hiding this comment

zsistla May 4, 2021

Choose a reason for hiding this comment

zsistla commented May 4, 2021

zsistla commented May 4, 2021

b-viguier commented May 6, 2021

zsistla commented May 6, 2021

zsistla commented May 6, 2021

zsistla commented May 7, 2021

zsistla commented May 12, 2021

zsistla commented May 12, 2021

b-viguier commented May 12, 2021

zsistla commented May 12, 2021

CLAassistant commented Apr 29, 2021 •

edited

Loading