Add support for tracking Redis client calls for non-Go programs #891
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #891 +/- ##
===========================================
- Coverage 77.57% 59.36% -18.21%
===========================================
Files 122 123 +1
Lines 8594 8737 +143
===========================================
- Hits 6667 5187 -1480
- Misses 1482 3181 +1699
+ Partials 445 369 -76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -567,6 +568,10 @@ func traceAttributes(span *request.Span, optionalAttrs map[attr.Name]struct{}) [ | |||
| request.ServerPort(span.HostPort), | |||
| } | |||
| case request.EventTypeSQLClient: | |||
| attrs = []attribute.KeyValue{ | |||
There was a problem hiding this comment.
I guess I have to add this for Kafka, right?
There was a problem hiding this comment.
Yes that's right, we need to specify which attributes will be ON by default for kafka. We tend to select by default the attributes which will not cause cardinality explosion. Kafka operation is definitely OK to add in metrics, I think topic is too, although there will be customers with many topics. For those customers we can recommend that they remove the attribute by changing the config.
| @@ -613,7 +639,7 @@ func spanKind(span *request.Span) trace2.SpanKind { | |||
| switch span.Type { | |||
| case request.EventTypeHTTP, request.EventTypeGRPC: | |||
| return trace2.SpanKindServer | |||
| case request.EventTypeHTTPClient, request.EventTypeGRPCClient, request.EventTypeSQLClient: | |||
| case request.EventTypeHTTPClient, request.EventTypeGRPCClient, request.EventTypeSQLClient, request.EventTypeRedisClient: | |||
There was a problem hiding this comment.
Same here, do we need this for Kafka?
| RedisClientDuration.Section: { | ||
| SubGroups: []*AttrReportGroup{&appAttributes, &appKubeAttributes}, | ||
| Attributes: map[attr.Name]Default{ | ||
| attr.DBOperation: true, |
There was a problem hiding this comment.
qq: is this what is defined here and that's why db.statement is hidden? Why we document hidden and what it means?
There was a problem hiding this comment.
It means that we don't include db.statement by default in metrics, because it will definitely cause high cardinality. We don't include it in traces by default too, because it might contain sensitive information, but if customers want it they can add it.
Added detection based on the format of the query and response. The protocol is in text format with various
operators.I added a new integration tests with OATS.
Relates to #879