Fix net.peer.* setting for Cassandra 4.+ #1482
Conversation
iNikem
requested review from
anuraaga,
jkwatson,
mateuszrzeszutek,
trask and
tylerbenson
as code owners
| @@ -47,8 +47,10 @@ protected InetSocketAddress peerAddress(CqlSession cqlSession) { | |||
| public void onResponse(Span span, ExecutionInfo executionInfo) { | |||
| Node coordinator = executionInfo.getCoordinator(); | |||
| if (coordinator != null) { | |||
| Optional<InetSocketAddress> address = coordinator.getBroadcastRpcAddress(); | |||
| address.ifPresent(inetSocketAddress -> NetPeerUtils.setNetPeer(span, inetSocketAddress)); | |||
| SocketAddress socketAddress = coordinator.getEndPoint().resolve(); | |||
There was a problem hiding this comment.
I don't think it's related to this PR since we have many instrumentations with the same pattern - I often worry about blocking an event loop with a DNS lookup just to set a span attribute. I want (someone) to remove the spec's language encouraging a DNS lookup just to resolve an attribute.
Just the word resolve makes it more obvious than Java where a seemingly normal getter has the chance to block and brought this point up for me :)
There was a problem hiding this comment.
Good point. But this may be related to the general question of bringing performance considerations into semantic conventions...
There was a problem hiding this comment.
I'd be interested if we force DNS resolution anywhere, I know we've tried to avoid that in the past.
In this case, I'm not sure why the method is named resolve(), at least the implementation I'm seeing when I look doesn't seem to resolve anything.
#1427 removed max java version to run Cassandra 4 tests. This allowed
testLatestDepjob to actually run them (as it uses java 11 to run). This exposed that starting from some version of Cassandra client driver the value returned bynode.getBroadcastRpcAddress()changed to0.0.0.0in our tests. This broke tests.But this also exposed that we used wrong address for
net.peer.*attributes in the first place.BroadcastRpcAddressmeans the address that Cassandra server accepts RPC connections. In case of containers/proxy this may be completely different address from the one used by client. Butnode.getEndPoint()returns "The information that the driver uses to connect to the node". Which seems like the correct one.Closes #1478
Closes #1473
Closes #1472
Closes #1461
Closes #1460