Skip to content

Commit

Permalink
fix(instrumentation-redis): Take host and port used for connection (#…
Browse files Browse the repository at this point in the history
…2072)

* fix(instrumentation-redis): Take host and port used for connection

The Redis client allows specifying connection options in several ways, with
sensible defaults. The following all translate into `127.0.0.1:6379`:

```js
createClient('redis://127.0.0.1:6379');
createClient({ host: '127.0.0.1', port: NaN });
createClient({})
createClient()
```

Redis somewhat normalises these separate options into its `options` member, and
stores the properties it uses to connect to the server in
`connection_options`. Examples of the difference between the two in the examples
preceding (slightly redacted for ease of reading):

```js
createClient('redis://127.0.0.1:6379');
// options = { port: '6379', host: '127.0.0.1' }
// connection_options = { port: 6379, host: '127.0.0.1', family: 4 }

createClient({ host: '127.0.0.1', port: NaN });
// options = { host: '127.0.0.1', port: NaN }
// connection_options = { port: 6379, host: '127.0.0.1', family: 4 }

createClient()
// options = { host: undefined }
// connection_options = { port: 6379, host: '127.0.0.1', family: 4 }
```

The instrumentation before this commit looks at the `options` property, which
contains caller-supplied values before they're fully normalised and smoothed
over by Redis. This means that for these weird cases, the instrumentation would
populate `NET_PEER_NAME` and `NET_PEER_PORT` with erroneous values.

This commit has the instrumentation the values the Redis client uses to connect
to the server, mirroring actual use.

* test(instrumentation-redis): Explicitly expect for a numeric port

---------

Co-authored-by: Amir Blum <[email protected]>
  • Loading branch information
Zirak and blumamir authored Apr 29, 2024
1 parent af2f3f1 commit 3ad9fdf
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
* limitations under the License.
*/
export interface RedisPluginClientTypes {
options?: {
host: string;
port: string;
connection_options?: {
port?: string;
host?: string;
};

address?: string;
Expand Down
6 changes: 3 additions & 3 deletions plugins/node/opentelemetry-instrumentation-redis/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ export const getTracedInternalSendCommand = (
);

// Set attributes for not explicitly typed RedisPluginClientTypes
if (this.options) {
if (this.connection_options) {
span.setAttributes({
[SEMATTRS_NET_PEER_NAME]: this.options.host,
[SEMATTRS_NET_PEER_PORT]: this.options.port,
[SEMATTRS_NET_PEER_NAME]: this.connection_options.host,
[SEMATTRS_NET_PEER_PORT]: this.connection_options.port,
});
}
if (this.address) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const memoryExporter = new InMemorySpanExporter();

const CONFIG = {
host: process.env.OPENTELEMETRY_REDIS_HOST || 'localhost',
port: process.env.OPENTELEMETRY_REDIS_PORT || '63790',
port: Number(process.env.OPENTELEMETRY_REDIS_PORT || 63790),
};

const URL = `redis://${CONFIG.host}:${CONFIG.port}`;
Expand Down

0 comments on commit 3ad9fdf

Please sign in to comment.