App crashes with a TypeError when using the Redis instrumentation
#2389
Comments
pichlermarc
added
priority:p1
|
That should be |
|
@rowanmanning I see how the error can happen and I'll come up with a fix. However, to reproduce this, I'm really curious what your Redis client config (or some other usage I'm missing) resulted in hitting this code path. The crash is because the catch here is being called: What's in your Redis client console.log('the err:', err);On line 32, just before the |
|
Sorry to jump-in here, @trentm. I think in the example @rowanmanning provided the value of |
|
Hi @trentm, thanks for jumping on this 🙂 If I add in a log before the The node -e "new URL('')"
We're adding OpenTelemetry to quite an old app which made some interesting decisions in the past that we can't really unpick easily. The URL is set like this in the real app: const redisUrl = process.env.REDIS_TLS_URL || process.env.REDIS_URL || '';
const redisOptions = { /* blah blah */ };
const client = redis.createClient({ url: redisUrl, ...redisOptions });Parts of the app can run without Redis, we seem to be relying on the existing behaviour of |
and
Thanks both! My bad for not reading the whole (excellent) bug report. I quickly jumped straight to the code when I saw the error stack. :) repro, and why the OTel code has an errorI can repro now. A thing that had been confusing me was how the but the OTel code has this guard: log spamWhile the crash is (will be, once there is a release) now fixed. There is still the log spam.
const { diag, DiagConsoleLogger, DiagLogLevel } = require('@opentelemetry/api');
diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.INFO);
const path = require('path');
const {NodeSDK} = require('@opentelemetry/sdk-node');
const {RedisInstrumentation} = require('./'); // This is `@opentelemetry/instrumentation-redis-4`, I was working in a git clone.
const sdk = new NodeSDK({
serviceName: path.parse(process.argv[1]).name,
instrumentations: [
new RedisInstrumentation()
],
});
process.once('beforeExit', async () => {
sdk.shutdown();
});
sdk.start();
const otel = require('@opentelemetry/api');
const redis = require('redis');
async function main() {
const client = redis.createClient({
url: '',
socket: {
port: '6379',
host: process.env.REDIS_HOST,
},
});
await client.connect();
await client.set('foo', 'bar');
const val = await client.get('bar');
console.log('GET bar:', val);
await client.quit();
}
const tracer = otel.trace.getTracer('test');
tracer.startActiveSpan('manual-parent-span', async (span) => {
await main();
span.end();
});Log spammy outputI'll do a follow-up PR that uses the same guard to avoid getting the error reporting at all for this case. |
…lient URL is the empty string Issue open-telemetry#2389 showed that this instrumentation would *crash* on a Redis client configured with `{url: ''}` (an empty string). The crash was fixed in open-telemetry#2397. This change avoids the once-crashy code, and hence the diag.error spam, by using the same guard before attempting to parse the configured client URL that the Redis client itself does, `if (options?.url)`, Refs: open-telemetry#2389
|
Thanks @trentm for the thorough explanation and quick resolution! |
…lient URL is the empty string (#2399) Issue #2389 showed that this instrumentation would crash on a Redis client configured with {url: ''} (an empty string). The crash was fixed in #2397. This change avoids the once-crashy code, and hence the diag.error spam, by using the same guard before attempting to parse the configured client URL that the Redis client itself does, if (options?.url), Refs: #2389
What version of OpenTelemetry are you using?
v0.52.1v0.49.1What version of Node are you using?
v20.16.0What did you do?
@metart43 and I tried to use the
redispackage alongside Node.js auto-instrumentations. We're connecting to Redis with an empty string as a URL which theredisclient accepts.What did you expect to see?
We expected to see a log (
Redis connected) and the app to continue to run.What did you see instead?
The app crashes with a
TypeError:Additional context
diagisundefinedby the time it reaches this lineThe text was updated successfully, but these errors were encountered: