Add MemcacheIntegration - #1981
Conversation
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
769361e to
760dcbf
Compare
pierotibou
left a comment
pierotibou
left a comment
There was a problem hiding this comment.
With one question mainly around type
| bool (*is_analytics_enabled)(void); | ||
| double (*get_sample_rate)(void); | ||
| void *aux[5]; | ||
| void *aux[7]; |
There was a problem hiding this comment.
Guess I could leave a comment :-D
| use DDTrace\Integrations\Guzzle\GuzzleIntegration; | ||
| use DDTrace\Integrations\Laravel\LaravelIntegration; | ||
| use DDTrace\Integrations\Lumen\LumenIntegration; | ||
| use DDTrace\Integrations\Memcached\MemcachedIntegration; |
There was a problem hiding this comment.
Why do we replace the old one? Shouldn't we just add the new one?
There was a problem hiding this comment.
I see, the refactoring functionality of PHPStorm replaced too many things. Well spotted. (And no, the new one should not be added, that's for PHP 5.)
|
|
||
| public function init() | ||
| { | ||
| if (!extension_loaded('memcache')) { |
There was a problem hiding this comment.
nitpicking
| if (!extension_loaded('memcache')) { | |
| if (!extension_loaded($NAME)) { |
There was a problem hiding this comment.
which $NAME? That's the literal name of the extension.
There was a problem hiding this comment.
You define a const NAME at the top, can't it be used here ?
There was a problem hiding this comment.
Because that's the name of the integration, not the name of the extension. (Also syntax is self::NAME)
| public function setCommonData(SpanData $span, $command) | ||
| { | ||
| $span->name = "Memcache.$command"; | ||
| $span->type = Type::MEMCACHE; |
There was a problem hiding this comment.
Should we really have a different type for memcache and memcached? We have 2 integrations for Redis in .NET that we both give a Redis type, same for GraphQL.
So I think we should stick with memcached
There was a problem hiding this comment.
I don't know. For db.system I have twice memcached. I can change it for type too if you want to.
There was a problem hiding this comment.
So no spec but we should use memcached, as it is what the agent expects (even though useless in that case). Cf https://cs.github.com/DataDog/datadog-agent/blob/9110db10f209e52ae630b9173aef8d54b770116b/pkg/trace/agent/obfuscate.go?q=ObfuscateMemcachedString#L71
|
|
||
| /** | ||
| * Add the servers to the span metadata. | ||
| * |
There was a problem hiding this comment.
What does that emoji mean?
There was a problem hiding this comment.
Thank you for the comments
There was a problem hiding this comment.
Oh, I just copied that comment from the MemcachedIntegration.
2 participants
Description
This adds an integration for Memcache, analogous to the Memcached integration we have.
Readiness checklist
Reviewer checklist