Skip to content
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

stats: Add RemoteAddr and LocalAddr to RPCStats #6897

Closed
pellared opened this issue Dec 28, 2023 · 6 comments
Closed

stats: Add RemoteAddr and LocalAddr to RPCStats #6897

pellared opened this issue Dec 28, 2023 · 6 comments
Assignees
Labels
Type: Feature New features or improvements in behavior

Comments

@pellared
Copy link
Contributor

pellared commented Dec 28, 2023

Use case(s) - what problem will this feature solve?

A stats.Handler implementation needs to capture the peer address in the HandleRPC.

The reason is that I want to add the gRPC client and server addresses as attributes to spans and metrics recorded by a stats handler.

For example imagine you would like to add the remote (peer, client) address as an additional attribute here:

// populateSpan populates span information based on stats passed in (invariants
// of the RPC lifecycle), and also ends span which triggers the span to be
// exported.
func populateSpan(ctx context.Context, rs stats.RPCStats, ti *traceInfo) {
if ti == nil || ti.span == nil {
// Shouldn't happen, tagRPC call comes before this function gets called
// which populates this information.
logger.Error("ctx passed into stats handler tracing event handling has no span present")
return
}
span := ti.span
switch rs := rs.(type) {
case *stats.Begin:
// Note: Go always added these attributes even though they are not
// defined by the OpenCensus gRPC spec. Thus, they are unimportant for
// correctness.
span.AddAttributes(
trace.BoolAttribute("Client", rs.Client),
trace.BoolAttribute("FailFast", rs.FailFast),
)
case *stats.PickerUpdated:
span.Annotate(nil, "Delayed LB pick complete")
case *stats.InPayload:
// message id - "must be calculated as two different counters starting
// from one for sent messages and one for received messages."
mi := atomic.AddUint32(&ti.countRecvMsg, 1)
span.AddMessageReceiveEvent(int64(mi), int64(rs.Length), int64(rs.CompressedLength))
case *stats.OutPayload:
mi := atomic.AddUint32(&ti.countSentMsg, 1)
span.AddMessageSendEvent(int64(mi), int64(rs.Length), int64(rs.CompressedLength))
case *stats.End:
if rs.Error != nil {
// "The mapping between gRPC canonical codes and OpenCensus codes
// can be found here", which implies 1:1 mapping to gRPC statuses
// (OpenCensus statuses are based off gRPC statuses and a subset).
s := status.Convert(rs.Error)
span.SetStatus(trace.Status{Code: int32(s.Code()), Message: s.Message()})
} else {
span.SetStatus(trace.Status{Code: trace.StatusCodeOK}) // could get rid of this else conditional and just leave as 0 value, but this makes it explicit
}
span.End()
}
}

Proposed Solution

Add RemoteAddr and LocalAddr fields to RPCStats.

Alternatives Considered

None

Additional Context

The gRPC server address attribute is recommended for all metrics by the current version of OpenTelemetry Semantic Conventions: https://opentelemetry.io/docs/specs/semconv/rpc/rpc-metrics/#attributes

Both the gRPC server and address attributes are recommended for all span by the current version of OpenTelemetry Semantic Conventions: https://opentelemetry.io/docs/specs/semconv/rpc/rpc-spans/

Here are some explorations: open-telemetry/opentelemetry-go-contrib#4539

@zasweq
Copy link
Contributor

zasweq commented Jan 10, 2024

Scaling up the RPCStats interface is not what we desire here. I think you should be able to pull out the peer with peer.FromContext, which will have the local address and remote address. Let me triage this though to see if this population of peer comes before all the stats handler callouts (I know some callouts will have it, but I don't know if all will).

@zasweq
Copy link
Contributor

zasweq commented Jan 17, 2024

Hello, peer.NewContext (which populates the context with a peer which contains both pieces of this information) comes before the TagConn call on the stats handler. Thus, you can use peer.FromContext to get this information in the stats handler. Let us know if you have any issues with this and we can reopen this issue, but this should work.

@zasweq zasweq closed this as completed Jan 17, 2024
@pellared
Copy link
Contributor Author

pellared commented Jan 29, 2024

@zasweq
I want to double-check one behavior.
Is it normal that for the gRPC Client I can only get the peer stats using peer.FromContext(ctx); for case *stats.OutHeader: (see: https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4873/files)?
Should it not work for each HandleRPC callback?
Is this behavior documented anywhere?

@zasweq
Copy link
Contributor

zasweq commented Jan 29, 2024

It works in every HandleRPC call, since it is populated before.

@pellared
Copy link
Contributor Author

It works in every HandleRPC call, since it is populated before.

I think does not for the client's stats handler. Should I create an issue with repro steps?

@pellared
Copy link
Contributor Author

@zasweq I created #6947

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

No branches or pull requests

2 participants