feat(agent): Memcached instance metrics with host name #958
Conversation
|
agent/php_internal_instrument.c
Outdated
| INTERNAL_FUNCTION_PARAM_PASSTHRU); | ||
| } | ||
|
|
||
| static nr_datastore_instance_t* nr_php_memcached_create_datastore_instance( |
There was a problem hiding this comment.
The redis equivalent of this function is in the file agent/php_redis.c. I am ambivalent on whether to create an equal agent/php_memcached.c for just this helper or to keep it here.
There was a problem hiding this comment.
I would vote for creating a php_memcached.c even if it's just for one function (for now) for future readability.
There was a problem hiding this comment.
refactored in a0d261f
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #958 +/- ##
==========================================
+ Coverage 78.32% 78.36% +0.03%
==========================================
Files 194 195 +1
Lines 26880 26935 +55
==========================================
+ Hits 21055 21108 +53
- Misses 5825 5827 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| segment = nr_segment_start(NRPRG(txn), NULL, NULL); | ||
| nr_segment_datastore_end(&segment, ¶ms); |
There was a problem hiding this comment.
This operation will probably result in an immediate (nanoseconds) segment duration. Is the rationale for this just that we have to send this data via a segment to the backend?
There was a problem hiding this comment.
Yes, the datastore metric needs to be send to the backend, and the length here isn't entirely important for several reasons. 1) this datastore metric is only an instance metric, and is not associated with any operation. 2) the operation being wrapped, add_servers, is not a function that the customer will likely care about timings of
There was a problem hiding this comment.
It sounds like we're going to be sending incorrect duration timings to the backend for this specific function call. I'm slightly worried that someone out there will care about these timings, but off the top of my head I can't think of a workaround that will generate all the segments we need in this manner without forcing a rewrite of our segment logic.
There was a problem hiding this comment.
It could call the metric creation function after the internal call and pass in the timing information to override the timings automatically generated by the segment start/end. It's not pretty, but would work
There was a problem hiding this comment.
Or potentially a small function that can loop over a set of segments that have already had timings set and update segment->start_time and segment->stop_time to appropriate values? Store start and stop as variables within the wrapper function then passing them as the values to the custom function?
I don't know if that would have any side-effects...
There was a problem hiding this comment.
But this does raise the important question: will these metrics -- even with correct timings -- confuse customers? There will be much longer instance metrics associated with all of the different datastore operations of the transaction, and then a few much shorter instance metrics at the start of each transaction that is not clear what they are
There was a problem hiding this comment.
Torn on this as well- would be good to see how this will be displayed in the UI to try and gauge the "confusion" factor
There was a problem hiding this comment.
A correction to my previous statement. These instance metrics (with the host and port) are not created for other database operations because we don't have access to the host and port at those times. So these instance metrics will ONLY be the short-lived metrics created when adding a server, and the longer database metrics will be separated under operation metrics.
| @@ -12,6 +12,36 @@ | |||
| #include "util_sql.h" | |||
| #include "util_logging.h" | |||
|
|
|||
| static bool create_instance_metric(nr_segment_t* segment, | |||
There was a problem hiding this comment.
could nix the bool return type entirely since it doesn't seem to get used in the below function calls
| require_once(realpath (dirname ( __FILE__ )) . '/memcache.inc'); | ||
|
|
||
| $memcached = new Memcached(); | ||
| $memcached->addServers(array( |
There was a problem hiding this comment.
Can you add a test case for the 0 port option that specifies a unix socket/localhost?
These metrics can be used by the backend to create AWS relationship maps with memcached