Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ioredis #558

Open
wants to merge 64 commits into
base: master
from
Open

Add ioredis #558

wants to merge 64 commits into from

Conversation

@naseemkullah
Copy link

naseemkullah commented Nov 21, 2019

Which problem is this PR solving?

Short description of the changes

  • Mostly copy pasted from opencensus ioredis plugin and opentelemetry redis plugin. Could not get tests to work locally: they seem to get stuck on assert.strictEqual(memoryExporter.getFinishedSpans().length, 1); Help/feedback appreciated, if not I can close this PR and let someone else do it properly.
@naseemkullah naseemkullah force-pushed the naseemkullah:ioredis branch 2 times, most recently from 350b8cc to 82549f8 Nov 21, 2019
@dyladan

This comment has been minimized.

Copy link
Contributor

dyladan commented Nov 21, 2019

@naseemkullah I will take a look at this. Typically that particular failure is a sign that the span is not properly created. Often it is because it is not yet ended.

@naseemkullah

This comment has been minimized.

Copy link
Author

naseemkullah commented Nov 21, 2019

Much appreciated @dyladan

@naseemkullah

This comment has been minimized.

Copy link
Author

naseemkullah commented Nov 21, 2019

I'm going to take this to Gitter probably, but does one run npm run docs-test locally? I installed yarn and lerna but still get

% npm run docs-test
internal/modules/cjs/loader.js:797
    throw err;
    ^

Error: Cannot find module '../lib/utils/unsupported.js'
Require stack:
- /usr/local/lib/node_modules/npm/bin/npm-cli.js
@dyladan

This comment has been minimized.

Copy link
Contributor

dyladan commented Nov 21, 2019

@naseemkullah no docs-test is typically only run in CI. It just makes sure there are no broken links before it is published. That said, they should be able to run locally so I'll take a look.

@naseemkullah naseemkullah force-pushed the naseemkullah:ioredis branch from 82549f8 to 5194ce1 Nov 21, 2019
@dyladan

This comment has been minimized.

Copy link
Contributor

dyladan commented Nov 21, 2019

@naseemkullah please try to avoid changing history on branches you've already pushed. it makes tracking changes harder

@naseemkullah

This comment has been minimized.

Copy link
Author

naseemkullah commented Nov 21, 2019

My apologies @dyladan

@naseemkullah

This comment has been minimized.

Copy link
Author

naseemkullah commented Nov 21, 2019

@naseemkullah no docs-test is typically only run in CI. It just makes sure there are no broken links before it is published. That said, they should be able to run locally so I'll take a look.

Pleas disregard this. My mistake, it works locally.

fix
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 22, 2019

Codecov Report

Merging #558 into master will increase coverage by 0.23%.
The diff coverage is 82.7%.

@@            Coverage Diff             @@
##           master     #558      +/-   ##
==========================================
+ Coverage   90.44%   90.67%   +0.23%     
==========================================
  Files         181      159      -22     
  Lines        9164     7839    -1325     
  Branches      814      719      -95     
==========================================
- Hits         8288     7108    -1180     
+ Misses        876      731     -145
Impacted Files Coverage Δ
...pentelemetry-plugin-ioredis/test/assertionUtils.ts 100% <100%> (ø)
packages/opentelemetry-plugin-ioredis/src/enums.ts 100% <100%> (ø)
...ckages/opentelemetry-plugin-ioredis/src/ioredis.ts 100% <100%> (ø)
...ges/opentelemetry-plugin-ioredis/test/testUtils.ts 15% <15%> (ø)
packages/opentelemetry-plugin-ioredis/src/utils.ts 78.57% <78.57%> (ø)
.../opentelemetry-plugin-ioredis/test/ioredis.test.ts 88.07% <88.07%> (ø)
...ges/opentelemetry-plugin-https/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
packages/opentelemetry-plugin-mysql/src/utils.ts 90.9% <0%> (-4.55%) ⬇️
...core/src/context/propagation/BinaryTraceContext.ts 96.22% <0%> (-2.11%) ⬇️
...telemetry-core/src/context/propagation/B3Format.ts 94.59% <0%> (-1.24%) ⬇️
... and 75 more
@mayurkale22

This comment has been minimized.

Copy link
Contributor

mayurkale22 commented Nov 22, 2019

/cc @vmarchaud (who wrote the original module), it would be awesome if you can review this.

@naseemkullah naseemkullah requested a review from vmarchaud Dec 5, 2019
@naseemkullah

This comment has been minimized.

Copy link
Author

naseemkullah commented Dec 5, 2019

@dyladan @markwolff @vmarchaud @obecny @mayurkale22

Fixed:

There always seems to be a cmd.promise defined even if ioredis is invoked with cb style.

After debugging/console logging, I noticed that the cb style invocations would end the span after a result was returned, but promise style invocations ended the span before the result was returned.

The cb style invocation tests required a nested cb to assert that the span ended AFTER the result was retrieved as follow

operation.method((err, _result) => {
assert.ifError(err);
(_result: string | number) => {
assert.strictEqual(memoryExporter.getFinishedSpans().length, 1);

... does that make sense?

Is it alarming that the promise style and cb style invocations of ioredis will end spans at slightly different times? If so can anyone offer up ideas as to what may be going on?

@vmarchaud

This comment has been minimized.

Copy link
Member

vmarchaud commented Dec 5, 2019

@naseemkullah That's because of how callback and promises works. Promises have their own internal queue (separate from the event loop) called the Microtask queue that is handled by V8.
Callbacks are just pointer to function and their lifecycle is handled by libuv which has a different scheduling mechanism.

Copy link
Member

vmarchaud left a comment

lgtm

@naseemkullah

This comment has been minimized.

Copy link
Author

naseemkullah commented Dec 5, 2019

@naseemkullah That's because of how callback and promises works. Promises have their own internal queue (separate from the event loop) called the Microtask queue that is handled by V8.
Callbacks are just pointer to function and their lifecycle is handled by libuv which has a different scheduling mechanism.

Very insightful, thanks @vmarchaud !

@mayurkale22 think we could get this one in

naseemkullah added 6 commits Dec 6, 2019
@naseemkullah naseemkullah requested a review from dyladan Dec 7, 2019
naseemkullah and others added 4 commits Dec 9, 2019
Reword sentence to read better

Co-Authored-By: Daniel Dyla <dyladan@users.noreply.github.com>
@naseemkullah naseemkullah requested a review from dyladan Dec 9, 2019
@naseemkullah

This comment has been minimized.

Copy link
Author

naseemkullah commented Dec 12, 2019

Sorry for the delay in getting the tests to pass. It's finally ready @mayurkale22

Copy link
Contributor

dyladan left a comment

Thanks for the work @naseemkullah this looks really good.

Everything implemented looks good to me, just have a few questions after looking over the ioredis api docs.

No mention of pipelining https://github.com/luin/ioredis/blob/master/README.md#pipelining
No mention of transactions https://github.com/luin/ioredis/blob/master/README.md#transaction
Can we add a test for scan streaming? https://github.com/luin/ioredis/blob/master/README.md#streamify-scanning
Does connecting create a span?
Does sentinel use the same sendCommand function? https://github.com/luin/ioredis/blob/master/README.md#sentinel
Cluster support? https://github.com/luin/ioredis/blob/master/README.md#cluster
What happens if you use lua scripting? https://github.com/luin/ioredis/blob/master/README.md#lua-scripting
It looks like there is no handling of pub/sub. Is this something we want to think about?

});

if (this.options) {
const { host, port } = this.options;

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 12, 2019

Contributor

If ioredis is initialized with no options, does it populate this.options with default values? Is there a way to get those values?

@naseemkullah

This comment has been minimized.

Copy link
Author

naseemkullah commented Dec 12, 2019

It is I who thank you for holding my hand through this @dyladan as well as @vmarchaud , @markwolff , @obecny and everyone else. This is really an amazing community and I appreciate the help and patience... I'll look into your comments ASAP.

On a side note, my only tests are the one in the test suite, I actually don't know how to run a redis client in some sample app and use the plugin i am working on to manually test. Is there documentation as how to do that? aka import the plugin i am working on into a sample app i have locally somewhere else.

@markwolff

This comment has been minimized.

Copy link
Member

markwolff commented Dec 12, 2019

@naseemkullah You could start with the redis example and port it over to use ioredis.
https://github.com/open-telemetry/opentelemetry-js/tree/master/examples/redis

Copy link
Contributor

mayurkale22 left a comment

lgtm

hrTimeToMicroseconds,
} from '@opentelemetry/core';

export const assertSpan = (

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Dec 13, 2019

Contributor

This is getting used by multiple plugins (grpc, redis and postgres), maybe later we should move this into common place.

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 13, 2019

Contributor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.