feat: add redis 4 connect span #1125
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
- Coverage 96.07% 93.83% -2.25%
==========================================
Files 14 17 +3
Lines 892 1135 +243
Branches 191 233 +42
==========================================
+ Hits 857 1065 +208
- Misses 35 70 +35
|
| return res | ||
| .then((result: unknown) => { | ||
| return new Promise(resolve => { | ||
| span.end(); | ||
| resolve(result); | ||
| }); | ||
| }) | ||
| .catch((error: Error) => { | ||
| return new Promise((_, reject) => { | ||
| span.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| message: error.message, | ||
| }); | ||
| span.end(); | ||
| reject(error); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I think this can be simplified to:
res.catch( (error: Error) => span.setStatus({
code: SpanStatusCode.ERROR,
message: error.message,
}).finally(() => span.end());
return res;There was a problem hiding this comment.
Actually, when thinking about it again, this might not be equivalent.
If this promise rejects and the user does not catch the error, then without instrumentation the app will crash. If instrumentation adds a catch block to the returned promise, then the app will no longer crash which is not desirable.
plugins/node/opentelemetry-instrumentation-redis-4/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| const span = plugin.tracer.startSpan( | ||
| `${RedisInstrumentation.COMPONENT}.connect`, |
There was a problem hiding this comment.
Instrumentation currently builds spans names with format:
`${RedisInstrumentation.COMPONENT}-${commandName}`Do we want to stay consistent here and use -? I am ok with changing it to dot in all places but it's a bit weird to have a mix from the same instrumentation IMO.
There was a problem hiding this comment.
I was doubting whether to use a . or - here (or something else). The actual redis commands have -, so I wanted to differentiate between a redis command and connect operation (which isn't a command). But I agree it looks somewhat inconsistent 🤔
There was a problem hiding this comment.
Actually, I am not even sure if the redis prefix is encouraged or not in semantic conventions
The span name SHOULD be set to a low cardinality value representing the statement executed on the database. It MAY be a stored procedure name (without arguments), DB statement without variable arguments, operation name, etc
The examples also suggest naming redis spans in like "HMSET myhash".
Anyway, this is not very relevant to this PR, so feel free to do what you think is best. We can revisit this in the future.
There was a problem hiding this comment.
For consistency I changed it to -, but indeed this might need another PR
| @@ -183,6 +185,54 @@ describe('redis@^4.0.0', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('client connect', () => { | |||
| it('produces a span', async () => { | |||
| const client = createClient({ | |||
There was a problem hiding this comment.
I suggest giving it another name. client is already defined in the global describe scope which can be confusing to read.
There was a problem hiding this comment.
Renamed the connection test clients
| const attributes = { | ||
| [SemanticAttributes.DB_SYSTEM]: DbSystemValues.REDIS, | ||
| [SemanticAttributes.NET_PEER_NAME]: options?.socket?.host, | ||
| [SemanticAttributes.NET_PEER_PORT]: options?.socket?.port, | ||
| [SemanticAttributes.DB_CONNECTION_STRING]: options?.url, | ||
| }; |
There was a problem hiding this comment.
This code is a duplication of the one in _traceClientCommand function.
WDYT about extracting it to utils (maybe call it getClientAttributes) and calling the function from both places?
There was a problem hiding this comment.
Good point, moved it to utils
plugins/node/opentelemetry-instrumentation-redis-4/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-redis-4/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
Which problem is this PR solving?
This PR adds a new span
redis.connectwhich is produced byclient.connect()call. This is a small nicety so that downstreamnetanddnsspans are properly attached to a redis span when initializing the connection, otherwise there would be 2 disjoint single span traces.Checklist
npm run test-all-versionsfor the edited package(s) on the latest commit if applicable.