feat: memcached instrumentation #539

rauno56 · 2021-06-17T12:14:40Z

Which problem is this PR solving?

Adds auto-instrumentation for memcached.

Short description of the changes

We're monkey-pathing an public generic command function all methods use internally.

codecov · 2021-06-17T12:24:20Z

Codecov Report

Merging #539 (4956469) into main (a8465a2) will decrease coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 4956469 differs from pull request most recent head 9fde92b. Consider uploading reports for the commit 9fde92b to get more accurate results

@@            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

Impacted Files	Coverage Δ
...telemetry-instrumentation-memcached/src/version.ts	`100.00% <0.00%> (ø)`
...y-instrumentation-memcached/src/instrumentation.ts	`96.42% <0.00%> (ø)`
...entelemetry-instrumentation-memcached/src/utils.ts	`81.25% <0.00%> (ø)`
...metry-instrumentation-memcached/test/index.test.ts	`93.47% <0.00%> (ø)`

plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts

vmarchaud · 2021-06-20T16:08:13Z

plugins/node/opentelemetry-instrumentation-memcached/src/types.ts

+import { InstrumentationConfig as BaseInstrumentationConfig } from '@opentelemetry/instrumentation';
+
+export interface InstrumentationConfig extends BaseInstrumentationConfig {
+  collectCommand?: boolean;


Could you use enhancedDatabaseReporting like other db instrumentation do (ex: mongodb) so its consistent ?

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! :)

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 ?

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.

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

Thanks for the info, @vmarchaud! 🙏 I renamed the config option.

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

plugins/node/opentelemetry-instrumentation-memcached/package.json

obecny · 2021-06-28T18:44:03Z

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 ?

rauno56 · 2021-06-29T13:05:25Z

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!

vmarchaud

👍

plugins/node/opentelemetry-instrumentation-memcached/package.json

plugins/node/opentelemetry-instrumentation-memcached/src/utils.ts

plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts

examples/memcached/tracer.js

blumamir · 2021-06-29T13:47:02Z

plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts

+      });
+
+      query.callback = api.context.bind(
+        context,


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.

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 ?

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

@vmarchaud - do you expect the callback to have the memcached span context? or the parent context?

parent context, we want to have the memcached context only within the internal code that does the request.

I agree, but current implementation is setting the Memcached span as context for the callback.
So do we agree to change it? @rauno56

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

@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

obecny

lgtm

vmarchaud · 2021-06-30T11:43:05Z

@rauno56 Should be good to merge after fixing the CI

rauno56 added 8 commits June 17, 2021 13:01

feat: first iteration of memcache instrumentation

ba4ebfa

feat: add memcached docker commands for tests

ab4cd1e

test: test collecting statements

0be9b2f

refactor: finish the instrumentation and the docs

5508b38

style: sort db configs in unit tests

8e6e290

test: add memcached service to tests

da68c7a

style: lint

d8f5a09

chore: update versions

584eb5a

rauno56 requested a review from a team June 17, 2021 12:14

rauno56 added 2 commits June 17, 2021 15:27

style: lint fix test utils

52bd8d6

fix: add env cars for memcached host and port for tests

d7defc1

johnbley reviewed Jun 17, 2021

View reviewed changes

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

rauno56 added 4 commits June 17, 2021 22:54

test: fix hardcoded localhost values from tests

37f1b6b

feat: rename the unnamed span to something more descriptive

a6d8e63

style: lint-fix

e8cbfac

docs: add a disclamer about the dangers of collectCommand

636b295

johnbley approved these changes Jun 18, 2021

View reviewed changes

vmarchaud reviewed Jun 20, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts Outdated Show resolved Hide resolved

vmarchaud reviewed Jun 20, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-memcached/src/instrumentation.ts Outdated Show resolved Hide resolved

vmarchaud reviewed Jun 20, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-memcached/package.json Outdated Show resolved Hide resolved

rauno56 and others added 5 commits June 28, 2021 12:22

Merge branch 'main' into feat/memcached-instrumentation

a95dcfc

refactor: use component logger

d2d3361

feat: update versions

b3cbb6a

Merge branch 'main' into feat/memcached-instrumentation

875f606

feat: default to {} in setConfig

9ec9897

rauno56 added 2 commits June 29, 2021 12:17

refactor: rename collecCommand configuration option

27a6921

feat: add example for memcached

4ef4fd2

vmarchaud approved these changes Jun 29, 2021

View reviewed changes

blumamir reviewed Jun 29, 2021

View reviewed changes

rauno56 added 2 commits June 29, 2021 17:08

fix: use semantic attribute for db.statement

607b31c

fix: use serviceName parameter

4668d9f

obecny approved these changes Jun 29, 2021

View reviewed changes

rauno56 added 2 commits June 30, 2021 14:24

test: test cb context

d92c8ce

feat: return to parent context in callback

9fde92b

blumamir approved these changes Jun 30, 2021

View reviewed changes

dyladan added the enhancement New feature or request label Jun 30, 2021

dyladan approved these changes Jun 30, 2021

View reviewed changes

dyladan merged commit c1b6eec into open-telemetry:main Jun 30, 2021

rauno56 deleted the feat/memcached-instrumentation branch June 30, 2021 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: memcached instrumentation #539

feat: memcached instrumentation #539

rauno56 commented Jun 17, 2021

codecov bot commented Jun 17, 2021 •

edited

Loading

vmarchaud Jun 20, 2021

rauno56 Jun 28, 2021

vmarchaud Jun 28, 2021

rauno56 Jun 28, 2021 •

edited

Loading

vmarchaud Jun 28, 2021

rauno56 Jun 29, 2021

blumamir Jun 29, 2021

obecny commented Jun 28, 2021

rauno56 commented Jun 29, 2021

vmarchaud left a comment

blumamir Jun 29, 2021

rauno56 Jun 29, 2021

vmarchaud Jun 29, 2021

blumamir Jun 29, 2021

vmarchaud Jun 29, 2021

blumamir Jun 29, 2021

blumamir Jun 29, 2021

obecny left a comment

vmarchaud commented Jun 30, 2021

feat: memcached instrumentation #539

feat: memcached instrumentation #539

Conversation

rauno56 commented Jun 17, 2021

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jun 17, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauno56 Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny commented Jun 28, 2021

rauno56 commented Jun 29, 2021

vmarchaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

vmarchaud commented Jun 30, 2021

codecov bot commented Jun 17, 2021 •

edited

Loading

rauno56 Jun 28, 2021 •

edited

Loading