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

feat: add grpc attribute "rpc.grpc.authority" #5151

Closed
wants to merge 1 commit into from
Closed

feat: add grpc attribute "rpc.grpc.authority" #5151

wants to merge 1 commit into from

Conversation

ralphgj
Copy link
Contributor

@ralphgj ralphgj commented Jan 17, 2022

No description provided.

@ralphgj ralphgj requested a review from a team as a code owner January 17, 2022 09:31
@@ -43,6 +43,7 @@
public <REQUEST, RESPONSE> ClientCall<REQUEST, RESPONSE> interceptCall(
MethodDescriptor<REQUEST, RESPONSE> method, CallOptions callOptions, Channel next) {
GrpcRequest request = new GrpcRequest(method, null, null);
request.setAuthority(next.authority());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javadocs for authority() say the following:

The authority of the destination this channel connects to. Typically this is in the format host:port.

Is there any significant difference between capturing authority() and the net.peer.* attributes?

@@ -17,6 +17,8 @@

@Nullable private volatile SocketAddress remoteAddress;

@Nullable private volatile String authority;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not change during this object's life, it's set once just after the construction - can you make it a final field instead?

@@ -14,7 +14,9 @@
final class GrpcAttributesExtractor implements AttributesExtractor<GrpcRequest, Status> {
@Override
public void onStart(AttributesBuilder attributes, GrpcRequest grpcRequest) {
// No request attributes
if (grpcRequest.getAuthority() != null) {
attributes.put(GrpcHelper.RPC_GRPC_AUTHORITY, grpcRequest.getAuthority());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new attribute does not seem to be present in the RPC semantic conventions spec. Would you mind opening a PR/issue against the spec repo that introduces this attribute?
And, for now, how about moving it to a separate AttributesExtractor that's disabled by default? We're using the experimental-span-attributes for these, e.g. otel.instrumentation.aws-sdk.experimental-span-attributes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I agree this attribute seems to already be captured as part of net.peer so unless there is a difference, we probably wouldn't want to go forward with even an experimental attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your reply. I found in grpc client spans there were no net.peer attributes. So I add the authority attribute by this PR and then we can know the invoked remote service's host and port. Is this a bug in grpc client instrumentation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ralphgj there should be net.peer attributes captured for grpc client spans since version 1.4.0 (#1338), if you're on latest and not seeing them it could be a bug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx @trask. I will close this PR and comment in #1338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants