stats/opentelemetry: separate out interceptors for tracing and metrics #8063
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8063 +/- ##
==========================================
+ Coverage 82.27% 82.38% +0.10%
==========================================
Files 386 387 +1
Lines 39038 39027 -11
==========================================
+ Hits 32120 32152 +32
+ Misses 5590 5569 -21
+ Partials 1328 1306 -22
|
janardhanvissa
force-pushed
the
refactor-tracing-metrics
branch
from
69df069 to
71804b4
Compare
|
@janardhanvissa its not clear what is the intention of this refactor. The follow up from opentelemetry tracing API PR was to create separate interceptors for metrics and traces. Right now, single interceptor is handling both trace and metrics options. Once we have separate unary and stream interceptor each for tracing and metrics, we don't have to check for options disabled/enabled everytime. |
There was a problem hiding this comment.
Please see this discussion https://github.com/grpc/grpc-go/pull/7852/files#r1909469701 and modify accordingly.
purnesh42H
changed the title
| type clientMetricsStatsHandler struct { | ||
| *clientStatsHandler | ||
| } | ||
| type clientTracingStatsHandler struct { |
There was a problem hiding this comment.
new line in between struct declaration
| method: h.determineMethod(method, opts...), | ||
| } | ||
| ctx = setCallInfo(ctx, ci) | ||
| if h.options.MetricsOptions.pluginOption != nil { |
There was a problem hiding this comment.
this metadata part is not applicable for tracing. It should only be in metrics interceptor. Please remove
| } | ||
| ctx = setCallInfo(ctx, ci) | ||
|
|
||
| if h.options.MetricsOptions.pluginOption != nil { |
There was a problem hiding this comment.
same. this metadata part is not applicable for tracing. It should only be in metrics interceptor. Please remove
| } | ||
|
|
||
| // perCallTraces records per call trace spans. | ||
| func (h *clientTracingStatsHandler) perCallTraces(_ context.Context, err error, _ time.Time, _ *callInfo, ts trace.Span) { |
There was a problem hiding this comment.
since we don't need time and callInfo, we should not just have them here instead of replacing them with _
| @@ -120,7 +120,17 @@ type MetricsOptions struct { | |||
| func DialOption(o Options) grpc.DialOption { | |||
| csh := &clientStatsHandler{options: o} | |||
| csh.initializeMetrics() | |||
| return joinDialOptions(grpc.WithChainUnaryInterceptor(csh.unaryInterceptor), grpc.WithChainStreamInterceptor(csh.streamInterceptor), grpc.WithStatsHandler(csh)) | |||
| var interceptors []grpc.DialOption | |||
| if o.isMetricsEnabled() { | |||
There was a problem hiding this comment.
i don't think we need to check anything here. We should just always add all the 4 interceptors. We should have initializeMetrics and initialTraces methods of respective stats handler that check and return early if they are not enabled.
Currently, initializeMetrics exist in statsHandler for server and client which does that. We should modify it to be part of metricsStatsHandler and create the similar function for traceStatsHandler
| @@ -140,7 +150,17 @@ var joinServerOptions = internal.JoinServerOptions.(func(...grpc.ServerOption) g | |||
| func ServerOption(o Options) grpc.ServerOption { | |||
| ssh := &serverStatsHandler{options: o} | |||
| ssh.initializeMetrics() | |||
| return joinServerOptions(grpc.ChainUnaryInterceptor(ssh.unaryInterceptor), grpc.ChainStreamInterceptor(ssh.streamInterceptor), grpc.StatsHandler(ssh)) | |||
| var interceptors []grpc.ServerOption | |||
| if o.isMetricsEnabled() { | |||
There was a problem hiding this comment.
same comment as dial option
| @@ -211,24 +218,41 @@ func (h *serverStatsHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) | |||
|
|
|||
| // HandleRPC implements per RPC tracing and stats implementation. | |||
| func (h *serverStatsHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) { | |||
| if h.options.isMetricsEnabled() { | |||
There was a problem hiding this comment.
we should have separate HandleRPC for metrics and trace statsHandlers instead of having common which requires to check what is enabled etc.
RELEASE NOTE: None