Update IORedis Plugin, fill dbinstance tag as host if condition.select doesn't exist.#73
Conversation
fix issue: apache/skywalking#7275
src/plugins/IORedisPlugin.ts
Outdated
| span.peer = host; | ||
| span.tag(Tag.dbType('Redis')); | ||
| span.tag(Tag.dbInstance(`${this.condition.select}`)); | ||
| span.tag(Tag.dbInstance(`${this.condition?.select}`)); |
There was a problem hiding this comment.
Could the condition be null? Could you share in which cases?
There was a problem hiding this comment.
As described in apache/skywalking#7275, here is what happened in my app:
There was a problem hiding this comment.
I got this. I mean is there a solution to make sure dbInstance would be set properly? Because this is an important tag, it is expected by the backend.
There was a problem hiding this comment.
As we use redis cluster, dbInstance makes no sense. Also, there's no condition property in redis cluster mode.
There was a problem hiding this comment.
For cluster mode, instance should be a cluster name or IP port list.
This is recommended.
There was a problem hiding this comment.
There is no cache dashboard to represent metrics for performance yet, but once it is added, this undefined name will show up in the cache server list.
So, only an endpoint name is not enough. I can see peer includes an IP:port. Isn't it for the Redis server?
There was a problem hiding this comment.
The Peer here stands for one of Redis cluster nodes. Is this value suitable for dbInstance ?
There was a problem hiding this comment.
I am expecting the whole IP1:port1, IP2:port2,...
There was a problem hiding this comment.
Redis cluster nodes cannot be acquired by the code contexts unless we call some function which I believe is inappropriate.
There was a problem hiding this comment.
Then at least use the peer value. It is at least better than null.
Use Peer value instand of undefined in cluster mode as @wu-sheng suggested. apache#73 (comment)
|
Maybe not perfect, but good for now. @kezhenxu94 WDYT? |
wu-sheng
changed the title
3 participants
fix issue: apache/skywalking#7275