feat: memcached instrumentation #539
Conversation
Codecov Report
@@ Coverage Diff @@
## main #539 +/- ##
==========================================
- Coverage 94.95% 94.91% -0.04%
==========================================
Files 162 166 +4
Lines 10102 10313 +211
Branches 1000 1026 +26
==========================================
+ Hits 9592 9789 +197
- Misses 510 524 +14
|
plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts
Show resolved
Hide resolved
| import { InstrumentationConfig as BaseInstrumentationConfig } from '@opentelemetry/instrumentation'; | ||
|
|
||
| export interface InstrumentationConfig extends BaseInstrumentationConfig { | ||
| collectCommand?: boolean; |
There was a problem hiding this comment.
Could you use enhancedDatabaseReporting like other db instrumentation do (ex: mongodb) so its consistent ?
There was a problem hiding this comment.
The name enhancedDatabaseReporting
- is not descriptive by itself and;
- sounds like it can affect multiple aspects of how the instrumentation works.
I'd personally stay away from such configuration options anywhere.
Happy to change it for you if that's a breaker though! :)
There was a problem hiding this comment.
We inherited this from Opencensus, i don't mind changing it but i would prefer to make it the same for all instrumentation anyway. Could you use enhancedDatabaseReporting and open an issue to discuss this ?
There was a problem hiding this comment.
Could you please share a reference to that in OpenCensus?
I think having config-parity is nice between instrumentations that are drop-in-replacements of each other or at least for the same library.
Since many of the auto-instrumentations will be provided by 3rd parties we can never ensure there is a finite list of options used across all instrumentations across all libs.
There was a problem hiding this comment.
Could you please share a reference to that in OpenCensus?
I was wrong that wasn't from Opencensus directly but from the google cloud tracer sdk (https://github.com/googleapis/cloud-trace-nodejs/search?q=enhancedDatabaseReporting.
You can lookup the initial proposal here: open-telemetry/opentelemetry-js#214
Since many of the auto-instrumentations will be provided by 3rd parties we can never ensure there is a finite list of options used across all instrumentations across all libs.
We cannot ensure consistency with 3rd party instrumentations but we can at least for 1st party instrumentation.
There was a discussion at the time here about this: open-telemetry/opentelemetry-js#614
There was a problem hiding this comment.
Thanks for the info, @vmarchaud! 🙏 I renamed the config option.
There was a problem hiding this comment.
I join the voice of not using enhancedDatabaseReporting parameter. Would be happy to see it replaced with meaningful names everywhere
plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-memcached/package.json
Outdated
Show resolved
Hide resolved
|
in general lgtm, I'm missing just one thing to be able to verify this easily, a working example so that I can generate spans myself, can you please add the example ? |
Added an example! |
plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| query.callback = api.context.bind( | ||
| context, |
There was a problem hiding this comment.
did not actually run it, but wouldn't setting the context this way make it so that every span started inside the callback will be the child of the (KIND=CLIENT) memcached span.
Not sure if it's intentional, but I think this is something we try to avoid in instrumentations.
There was a problem hiding this comment.
Yeah, I was thinking about it as well while implementing it. In some cases it makes sense, in others, not.
What's the correct way to implement it, @vmarchaud , @obecny ?
There was a problem hiding this comment.
Since you use .with when calling wrapQueryCompiler you shouldn't need to .bind here (except if the memcached client is loosing context, for example if they have internal queues), if it works without i believe can you can remove
There was a problem hiding this comment.
@vmarchaud - do you expect the callback to have the memcached span context? or the parent context?
There was a problem hiding this comment.
parent context, we want to have the memcached context only within the internal code that does the request.
There was a problem hiding this comment.
I agree, but current implementation is setting the Memcached span as context for the callback.
So do we agree to change it? @rauno56
There was a problem hiding this comment.
Since you use
.withwhen callingwrapQueryCompileryou shouldn't need to .bind here (except if the memcached client is loosing context, for example if they have internal queues), if it works without i believe can you can remove
@vmarchaud sorry I was reading your reply without full attention. I think it should still be binded, but to parent context, but not sure about it. better to try and see how it behaves
|
@rauno56 Should be good to merge after fixing the CI |
Which problem is this PR solving?
Adds auto-instrumentation for
memcached.Short description of the changes
We're monkey-pathing an public generic
commandfunction all methods use internally.