redisotel: Missing server.address from sentinel traces

[AndrewWinterman]

We are using sentinel for high availability.

I'm in process of writing a library to support switchover from legacy deployments which do not use sentinel, and I noticed the server.address is missing from the sentinel spans:

Initialization of the tracer looks as follows:

	failoverOptions := &redis.FailoverOptions{
		SentinelAddrs:    []string{cfg.SentinelAddr},
		ClientName:       app.Info().PodID,
		DB:               cfg.DBNumber,
		Password:         string(cfg.Password),
		SentinelPassword: string(cfg.Password),

		// Base number of socket connections.
		// Default is 10 connections per every available CPU as reported by runtime.GOMAXPROCS.
		// If there is not enough connections in the pool, new connections will be allocated in excess of PoolSize,
		// you can limit it through MaxActiveConns
		PoolSize: cfg.PoolSize,
		// Minimum number of idle connections which is useful when establishing
		// new connection is slow.
		// Default is 0. the idle connections are not closed by default.
		MinIdleConns: cfg.MinIdleConns,
		// Maximum number of idle connections.
		// Default is 0. the idle connections are not closed by default.
		MaxIdleConns: cfg.MaxIdleConns,
		// Maximum number of connections allocated by the pool at a given time.
		// When zero, there is no limit on the number of connections in the pool.
		MaxActiveConns: cfg.MaxActiveConns,

		// there are additional idle
		ReplicaOnly:           role == RedisReplicaRoleReader,
		MasterName:            cfg.LeaderName,
		ContextTimeoutEnabled: true,

		// <<Stencil::Block(failoverOptions)>>

		// <</Stencil::Block>>
	}

	redis.SetLogger(&redisLogger{})

	var client redis.UniversalClient

	switch role {
	case RedisReplicaRoleReader:
		failoverOptions.RouteByLatency = cfg.RouteReadsByLatency
		client = redis.NewFailoverClusterClient(failoverOptions)
	case RedisReplicaRoleWriter:
		client = redis.NewFailoverClient(failoverOptions)
	default:
		return nil, fmt.Errorf("unknown RedisReplicaRole: %v", role)
	}

	ping := client.Ping(ctx)
	if ping.Err() != nil {
		return nil, errors.Wrapf(ping.Err(), "failed to contact Redis Sentinel at %q", cfg.SentinelAddr)
	}

	if err := redisotel.InstrumentTracing(client); err != nil {
		return nil, errors.Wrap(err, "failed to instrument Redis tracing")
	}
	if err := redisotel.InstrumentMetrics(client); err != nil {
		return nil, errors.Wrap(err, "failed to instrument Redis tracing")
	}
	return client, nil

Related:

• PR: add server address and port span attributes to redis otel trace instrumentation #2826
• Older issue adding it for standalone clients: redisotel: Missing server address and port attributes in spans #2825

Cause is because of this line here:

https://github.com/redis/go-redis/blob/master/sentinel.go#L90

It uses "FailoverClient" instead of any real address.

[AndrewWinterman]

What would the ramifications of simply returning the original or current leader address from the Options call be? https://github.com/redis/go-redis/blob/master/sentinel.go#L90

[ndyakov]

Hello @AndrewWinterman , thanks for reporting this. Will take the time next week to look more into it.

[ndyakov]

Actually, maybe @monkey92t or @vmihailenco can help you with this. I see it is related to redisotel and not go-redis

[vmihailenco]

I believe this is the limitation of the NewFailoverClient client, which is not a separate client but just a wrapper over *redis.Client, i.e. NewFailoverClientreturns*redis.Clientwith a fakeFailoverClient` addr.

It uses "FailoverClient" instead of any real address.

You're right. We can't use the real address, because we don't have it and only retrieve it later from the Sentinel. The address can change over time / during failover.

And there is no way we can retrieve the current addr from the Cmd.

[AndrewWinterman]

I suppose my expectation is that it uses the list or adds the address after the fact. Not sure if that's possible with the setup