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

[ioredis] No traces #668

Closed
naseemkullah opened this issue Jan 5, 2020 · 18 comments · Fixed by #714
Closed

[ioredis] No traces #668

naseemkullah opened this issue Jan 5, 2020 · 18 comments · Fixed by #714
Assignees
Labels
bug Something isn't working

Comments

@naseemkullah
Copy link
Member

naseemkullah commented Jan 5, 2020

Please answer these questions before submitting a bug report.

What version of OpenTelemetry are you using?

0.3.2

What version of Node are you using?

12.x.x

What did you do?

const { JaegerExporter } = require('@opentelemetry/exporter-jaeger');
const opentelemetry = require('@opentelemetry/core');
const { NodeTracer } = require('@opentelemetry/node');
const { SimpleSpanProcessor } = require('@opentelemetry/tracing');

const tracer = new NodeTracer();
const exporter = new JaegerExporter({ serviceName: 'redis-tester' });
tracer.addSpanProcessor(new SimpleSpanProcessor(exporter));

// Initialize the tracer
opentelemetry.initGlobalTracer(tracer);

const Redis = require('ioredis');

const redisOptions = {
  host: process.env.REDIS_HOST || 'localhost',
  port: process.env.REDIS_PORT || 6379,
};

const redis = new Redis(redisOptions)
redis.set('test', 'data');

setInterval(async () => {
  const value = await redis.get('test')
  console.log(value);
}, 1000);

What did you expect to see?

traces in Jaeger

What did you see instead?

Nothing

Additional context

output of command:

node index.js
PluginLoader#load: trying loading [email protected]
PluginLoader#load: applying patch to [email protected] using @opentelemetry/plugin-ioredis module
data
data
data
data
data
[...]

Does this have anything to do with no parentSpan existing when redis is called/not called within an http route?

@naseemkullah naseemkullah added the bug Something isn't working label Jan 5, 2020
@naseemkullah naseemkullah changed the title [ioredis] No traces [ioredis] No traces if not called within an http route Jan 5, 2020
@naseemkullah naseemkullah changed the title [ioredis] No traces if not called within an http route [ioredis] No traces Jan 5, 2020
@dyladan
Copy link
Member

dyladan commented Jan 5, 2020

Even if a plugin starts a span with no parent (e.g. a root span) it should generate a trace. I'll try to reproduce this monday morning.

@dyladan
Copy link
Member

dyladan commented Jan 5, 2020

@naseemkullah i'm assigning both of us to this since you wrote the plugin.

@OlivierAlbertini
Copy link
Member

Is this happens with Zipkin ?

@naseemkullah
Copy link
Member Author

Is this happens with Zipkin ?

Yep!
It's an issue with the ioredis plugin 😞

@dyladan
Copy link
Member

dyladan commented Jan 6, 2020

@naseemkullah the "supported versions" specifier is ^2.0.0. The current version of ioredis is 4.14.1. Please update the ioredis version specifier and test with 3 and 4.

@mayurkale22
Copy link
Member

At first glance, looks like supported version for ioredis is ^2.0.0 (https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-ioredis/src/ioredis.ts#L25) but the installed version is 4.14.1.

@mayurkale22
Copy link
Member

@dyladan you beat me by few seconds :)

@naseemkullah
Copy link
Member Author

naseemkullah commented Jan 7, 2020

Thanks for pointing this out. I'm not sure how to test for version 3.x.x and 4.x.x

I've installed versions 2.x.x, 3.x.x and 4.x.x in the plugin folder itself and ran npm run test:local successfully, not sure why that is.

Furthermore, I tried running the example in the ioredis example PR with ioredis 2.x.x but got:

.../open-telemetry/opentelemetry-js/examples/ioredis/index.js:7
const redis = new Redis();
              ^

TypeError: Redis is not a constructor

which I assume has something to do with the redis types package in the plugin? We should probably remove ^2.0.0 as a supported version, I guess?

Finally, I do not know how to test an updated version of the package with versions ^3.0.0 and ^4.0.0 added as supported versions. Please advise.

@naseemkullah
Copy link
Member Author

In any case #671 created

@dyladan
Copy link
Member

dyladan commented Jan 7, 2020

The tests pass because they do not depend on the plugin loader. The plugin loader is what is preventing the plugin from being loaded when the version specifier doesn't match in a regular application. In the tests, you explicitly enable() the plugin regardless of the version of ioredis installed as a dependency.

I would install ioredis version 3.x.x and 4.x.x in the example and make sure it works as expected, but it seems like the current plugin is actually targeting 4.x.x not 2.x.x anyway.

@naseemkullah
Copy link
Member Author

The tests pass because they do not depend on the plugin loader. The plugin loader is what is preventing the plugin from being loaded when the version specifier doesn't match in a regular application. In the tests, you explicitly enable() the plugin regardless of the version of ioredis installed as a dependency.

Ah I see, thanks for explaining.

I would install ioredis version 3.x.x and 4.x.x in the example and make sure it works as expected, but it seems like the current plugin is actually targeting 4.x.x not 2.x.x anyway.

I've installed 3.x.x and 4.x.x in the example but the plugin does not patch as it does with 2.x.x, which errors out with TypeError: Redis is not a constructor... how does the current plugin target 4.x.x ? I thought it targetted the supportedVersion of 2.x.x

@naseemkullah
Copy link
Member Author

Furthermore, how can I have the example override the ioredis-plugin in npm, with one in my local repo (where I edit the supportedVersions) ? I tried running lerna link but that did not seem to do it

@dyladan
Copy link
Member

dyladan commented Jan 7, 2020

You don't use link, but actually put the filepath directly in the package.json. So you would have:

{
  dependencies: {
    "@opentelemetry/plugin-ioredis": "../../path/to/plugin"
  }
}

@naseemkullah
Copy link
Member Author

Thanks @dyladan , As for:

const redis = new Redis();
              ^

TypeError: Redis is not a constructor

which appears regardless of ioredis version used by main application, I've tried adding "esModuleInterop": true and then also "allowSyntheticDefaultImports": true to the tscfonfig file which did not seem to help. Any other ideas?

@dyladan
Copy link
Member

dyladan commented Jan 8, 2020

I think it is probably a simple matter of using the ioredis module incorrectly. If you're changing the ioredis module version, but not the @types/ioredis, then they probably don't match.

@naseemkullah
Copy link
Member Author

I am changing the ioredis version in the main/example app, which does not have @types/ioredis.

@types/ioredis and ioredis in the plugin stay the same (both at latest available versions).

@naseemkullah
Copy link
Member Author

This should be fixed by #714 when merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants