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

fix: add attributes of net.peer.* for grpc client span #5324

Merged
merged 1 commit into from
Feb 10, 2022
Merged

fix: add attributes of net.peer.* for grpc client span #5324

merged 1 commit into from
Feb 10, 2022

Conversation

ralphgj
Copy link
Contributor

@ralphgj ralphgj commented Feb 9, 2022

Try to resolve #5178

@ralphgj ralphgj requested a review from a team February 9, 2022 03:16
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

nice! I didn't notice that we had a repro for this issue in the tests themselves 👍

@@ -56,7 +58,8 @@
}
}

SocketAddress address = result.getAttributes().get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR);
URI uri = GrpcUtil.authorityToUri(next.authority());
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

SocketAddress address = null;
try {
URI uri = new URI(null, next.authority(), null, null, null);
address = new InetSocketAddress(uri.getHost(), uri.getPort() == -1 ? 80 : uri.getPort());
Copy link
Member

Choose a reason for hiding this comment

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

Won't calling the InetSocketAddress constructor cause DNS resolution here? Should we use InetSocketAddress.createUnresolved() instead?
On the other hand - it's a client instrumentation, so in theory the address should already be resolved - is that the case here? Or is the intercept method called earlier than that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand - it's a client instrumentation, so in theory the address should already be resolved - is that the case here? Or is the intercept method called earlier than that?

Yes, I found that invoked result.getAttributes().get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR) will return null. But in TracingClientCallListener#onMessage method, invoked it will get the expected result. So the root cause should be result.getAttributes() called too early.

Copy link
Member

Choose a reason for hiding this comment

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

@ralphgj I think the concern here is that calling new InetSocketAddress will trigger the host name to be resolved to an IP address, which we try to avoid triggering from instrumentation

InetSocketAddress.createUnresolved() is probably safer unless we are clear this won't be a problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I tried and then found the attributes in spans without net.peer.ip. Shall we go on using InetSocketAddress.createUnresolved() instead?

Copy link
Member

Choose a reason for hiding this comment

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

ya, it's ok not to capture net.peer.ip

@trask trask merged commit e551d99 into open-telemetry:main Feb 10, 2022
@trask
Copy link
Member

trask commented Feb 10, 2022

thx @ralphgj!

@ralphgj ralphgj deleted the fix-grpc-client-attributes branch February 11, 2022 03:18
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
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.

Grpc client spans doesn't capture the attributes of net.peer.*
4 participants