-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix histogram counter for per RPC counts #151
Conversation
On receiving on sending streams recorded multiple values of 1 per RPC. Use the state counters to accumulate the messages and report total per RPC.
attribute.Key(rpcBufConnectStatusCode).String("data_loss"), | ||
), | ||
Count: 1, | ||
Sum: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should the handler under error respond? Kept the current behaviour, no response count is reported as no message is sent (only error).
@@ -649,6 +651,7 @@ func TestStreamingMetricsFail(t *testing.T) { | |||
semconv.RPCSystemKey.String(bufConnect), | |||
semconv.RPCServiceKey.String(pingv1connect.PingServiceName), | |||
semconv.RPCMethodKey.String(PingStreamMethod), | |||
attribute.Key(rpcBufConnectStatusCode).String("data_loss"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Includes attributes from error status.
@@ -247,7 +245,13 @@ func (i *Interceptor) WrapStreamingClient(next connect.StreamingClientFunc) conn | |||
span.SetAttributes(headerAttributes(protocol, responseKey, conn.ResponseHeader(), i.config.responseHeaderKeys)...) | |||
span.SetStatus(spanStatus(protocol, state.error)) | |||
span.End() | |||
instrumentation.duration.Record(ctx, i.config.now().Sub(requestStartTime).Milliseconds(), metric.WithAttributes(state.attributes...)) | |||
instrumentation.requestsPerRPC.Record(ctx, state.sentCounter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importantly requests
are sendCounter
here and below for the handler the reverse.
On receiving or sending streams we currently recorded multiple values of 1 per RPC. Instead use the state counters to accumulate the number of messages and report the sum total per RPC. See https://opentelemetry.io/docs/specs/semconv/rpc/rpc-metrics/
Changes
rpc.server.requests_per_rpc
andrpc.server.responses_per_rpc
are reported at the end of the RPC to reflect the total count of messages per RPC