fix(redis): Add instance to Redis DB context in ioredis.js

[sibelius]

solves #4857

Checklist

• Implement code
• Add tests
• Update TypeScript typings
• Update documentation
• Add release notes to docs/release-notes/index.md
• Commit message follows commit guidelines

[github-actions]

🤖 GitHub comments

Just comment with:

• run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

[sibelius]

we tested this in production and it is working well

[sibelius]

any plans to review or land this?

what are the missing parts?

[trentm]

Elastic's APM agent spec for Redis spans (https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-db.md#redis) says that no value should be used for context.db.instance, so it is difficult to accept this change.

Looking for possible inspiration from OpenTelemetry Semantic Conventions, the span attribute most similar to Elastic APM's span.context.db.name is db.name, which fairly recently was changed to db.namespace in the recent stabilization of DB Semantic Conventions: https://opentelemetry.io/docs/specs/semconv/database/redis/#spans
The specification there is that the Redis database index, if anything, should be used for that field. So that doesn't help here.

The screenshots you showed with "redis" are the dependencies table in Kibana's APM UI? I'm pretty sure that this is from the value of service.target.type and service.target.name attributes of the span. These are typically automatically set from the DB context that your patch is setting:

apm-agent-nodejs/lib/instrumentation/span.js

Lines 185 to 203 in 2cec62a

	Span.prototype._inferServiceTargetAndDestinationService = function () {
	// `context.service.target.*` must be set for exit spans. There is a public
	// `span.setServiceTarget(...)` for users to manually set this, but typically
	// it is inferred from other span fields here.
	// https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans-service-target.md#field-values
	if (this._excludeServiceTarget || !this._exitSpan) {
	this._serviceTarget = null;
	} else {
	if (!this._serviceTarget) {
	this._serviceTarget = {};
	}
	if (!('type' in this._serviceTarget)) {
	this._serviceTarget.type =
	this.subtype || this.type || constants.DEFAULT_SPAN_TYPE;
	}
	if (!('name' in this._serviceTarget)) {
	if (this._db) {
	if (this._db.instance) {
	this._serviceTarget.name = this._db.instance;

Another way to set those service.target.* values is to use the span.setServiceTarget(...) API. However, that doesn't help because there is no effective place in your user-code to make that call.

Really, I think the feature wanted here is a separation of dependencies by host (server.address in OpenTelemetry terms). Presumably one would want this to be opt-in: some DB setups involve a pool of endpoints that logically are a single dependency. I'm not sure if this functionality would best be an agent-side or server-side thing.

At best we could only add this functionality behind a configuration option.

I wonder if using apm.addSpanFilter(...) would be possible to workaround the limitation. The span "payload" you get as the first argument that function is not the class Span, but a serialized span object something like this:

{
    "name": "HSET",
    "type": "db",
    "id": "828356d8e28c47e0",
    "transaction_id": "06544993fc6dbbfe",
    "parent_id": "06544993fc6dbbfe",
    "trace_id": "2373a3a7f65dd27ea537d14c18ab4312",
    "subtype": "redis",
    "action": "query",
    "timestamp": 1763595378528182,
    "duration": 0.881,
    "context": {
        "service": {
            "target": {
                "type": "redis"
            }
        },
        "destination": {
            "address": "localhost",
            "port": 6379,
            "service": {
                "type": "",
                "name": "",
                "resource": "redis"
            }
        },
        "db": {
            "type": "redis"
        }
    },
    "sync": false,
    "outcome": "success",
    "sample_rate": 1
}

So a potential (untested) span filter would be:

apm.addSpanFilter(span => {
    if (span.type === 'db' && span.context?.service?.target?.type === 'redis') {
        const addr = span.context.destination?.address;
        if (addr) {
            span.context.service.target.name = addr;
        }
    }
    return span;
});

Are you able to give that a try?

[trentm]

If that does NOT work, it is possible that I am mistaken that the span.context.service.target.{name,type} are the relevant fields, and that the span.context.destination.service.resource field needs to change to something like redis/${addr}. I would need to dig into that.

[trentm]

that the span.context.destination.service.resource field needs to change

In fact you can see both of those fields having changed in the test failure output:

     expected: |-
      { service: { target: { type: 'redis' } }, destination: { address: 'localhost', port: 6379, service: { type: '', name: '', resource: 'redis' } }, db: { type: 'redis' } }
    actual: |-
      { service: { target: { type: 'redis', name: 'localhost' } }, destination: { address: 'localhost', port: 6379, service: { type: '', name: '', resource: 'redis/localhost' } }, db: { type: 'redis', instance: 'localhost' } }

So I think it would be good to have the addSpanFilter function also change the .destination. field. Something like:

apm.addSpanFilter((span) => {
  if (span.type === 'db' && span.context?.service?.target?.type === 'redis') {
    const addr = span.context.destination?.address;
    if (addr) {
      span.context.service.target.name = addr;
      const destRes = span.context.destination.service?.resource;
      if (destRes) {
        span.context.destination.service.resource = destRes + '/' + addr;
      }
    }
  }
  return span;
});

[sibelius]

I just copy and paste the mongodb approach

[trentm]

I just copy and paste the mongodb approach

I may be misunderstanding you, but the mongodb instrumentation is using the MongoDB database name, not the server address/host, for the DB "instance" field.

• implementation: https://github.com/elastic/apm-agent-nodejs/blob/v4.15.0/lib/instrumentation/modules/mongodb.js#L171
• spec: https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-db.md#mongodb

[sibelius]

you are correct

but for redis you usually have a single database per redis instance