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

Net attributes getters changes (in preparation for HTTP spec impl) #6503

Merged
merged 7 commits into from
Sep 12, 2022

Conversation

mateuszrzeszutek
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek commented Aug 23, 2022

I started working on implementing the most recent HTTP spec changes, and I think I ran into several problems with the current shape of our net.* attributes extractors.

  • I added a couple of methods for determining the host name/port/socket to the NetServerAttributesGetter (I think these will be needed in the RPC semconv too)
  • I looked through most of our instrumentations, and I came to a conclusion that in most places the InetSocketAddress retrieved from the library code actually represents the socket address, not the logical name/port. It is especially true for most of the HTTP instrumentations, since in these the peer name/port is supposed to be extracted from the URL, not from the socket. I decided to change the InetSocket... classes so that they only extract net.sock.* attributes from the InetSocketAddress classes, and leave peerName/peerPort for the user to implement.

@trask @lmolkova please take a look at this draft - I'm quite interested in your opinion on the InetSocketAddress changes in particular.

(the 2nd commit contains the actual changes, the first one is just cleanup)

@lmolkova
Copy link
Contributor

lmolkova commented Aug 24, 2022

Looks good!

One thing I'm not sure about is keeping logical net.peer.name, net.peer.port (and host pair) in NetAttribute*.
E.g. unix domain socket instrumentation would never have logical attributes but would have to implement methods anyway.

The alternative is something like LogicalPeerAttribute*, LogicalHostAttribute* interfaces - HTTP and other conventions would implement them directly. gRPC might be tricky since it can work over local sockets.

I don't have a strong opinion, methods are already there and more abstractions won't make life easier. I also assume that every network-related semantic convention would need logical attributes, so current approach in this PR is fine too.

@mateuszrzeszutek
Copy link
Member Author

The alternative is something like LogicalPeerAttribute*, LogicalHostAttribute* interfaces - HTTP and other conventions would implement them directly. gRPC might be tricky since it can work over local sockets.

I don't have a strong opinion, methods are already there and more abstractions won't make life easier. I also assume that every network-related semantic convention would need logical attributes, so current approach in this PR is fine too.

Hmm, I think I'll leave them in the Net*Getter interfaces for now; some of the socket attributes have conditions that depend on logical attributes, e.g. net.sock.peer.addr is recommended "If available and different from net.peer.name and if net.sock.peer.addr is set."

@mateuszrzeszutek mateuszrzeszutek force-pushed the net-attributes branch 23 times, most recently from a46d26a to 312cc62 Compare August 30, 2022 10:23
@mateuszrzeszutek mateuszrzeszutek marked this pull request as ready for review August 30, 2022 15:41
@mateuszrzeszutek mateuszrzeszutek requested a review from a team August 30, 2022 15:41
@mateuszrzeszutek
Copy link
Member Author

@open-telemetry/java-instrumentation-maintainers this PR is ready for review. It is absurdly huge though (net semconv is used in the vast majority of all instrumentations after all) -- I tried to split it into 3 commits (instrumentation-api changes, actual getter implementation changes, fixing tests) to make it more readable, but if you have any alternative idea please let me know -- I'll try making this PR more manageable.

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.

will keep reviewing, just wanted to leave first comments

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.

a couple more comments, sorry for the pace..

Comment on lines -26 to -30
// dubbo 3 doesn't set remote address for client calls
if (address == null) {
URL url = request.url();
address = InetSocketAddress.createUnresolved(url.getHost(), url.getPort());
}
Copy link
Member

Choose a reason for hiding this comment

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

this is a nice example where the new semantic conventions cleared things up

}
}

requestInfo.setPeerAddress(channelHandlerContext.channel().remoteAddress());
Copy link
Member

Choose a reason for hiding this comment

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

nice

@mateuszrzeszutek
Copy link
Member Author

a couple more comments, sorry for the pace..

No problem at all! This PR is so huge that I don't expect anybody to review it in one go 🙈 😅

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.

😅 that was not the straight-forward refactoring I was expecting, thx for going through all of those instrumentations to map them correctly

if (remoteAddress instanceof Inet6Address) {
return "inet6";
}
return null;
Copy link
Member

Choose a reason for hiding this comment

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

@lmolkova should we report inet when that is known? the spec says we don't need to, but it feels like we may be losing something by not reporting it?

Copy link
Member

Choose a reason for hiding this comment

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

I think I understand now.

It's conditionally required

If different than inet and if any of net.sock.peer.addr or net.sock.host.addr are set. Consumers of telemetry SHOULD accept both IPv4 and IPv6 formats for the address in net.sock.peer.addr if net.sock.family is not set. This is to support instrumentations that follow previous versions of this document.

So if consumers see net.sock.peer.addr or net.sock.host.addr, and they do not see any net.sock.family, then they are allowed to assume that the net.sock.family is inet.

if (remoteAddress == null) {
remoteAddress = request.remoteAddressOnStart();
public String peerName(NettyConnectionRequest request, @Nullable Channel channel) {
SocketAddress requestedAddress = request.remoteAddressOnStart();
Copy link
Member

Choose a reason for hiding this comment

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

it seems like all netty operations are at socket layer, can you clarify why this works for net.peer.name?

Copy link
Member Author

Choose a reason for hiding this comment

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

My reasoning here was that in the establishing connection phase, the socket address passed at the start (before the DNS lookup & connection occurs) is actually a logical address

@trask
Copy link
Member

trask commented Sep 11, 2022

@mateuszrzeszutek any reason not to merge this for 1.18.0?

@trask trask added this to the v1.18.0 milestone Sep 11, 2022
@mateuszrzeszutek
Copy link
Member Author

If you're fine with it then I'm fine with it too 👍

@trask trask merged commit cfdbe75 into open-telemetry:main Sep 12, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the net-attributes branch September 13, 2022 09:12
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
…pen-telemetry#6503)

* Net attributes getters changes: instrumentation-api-semconv changes

* Net attributes getters changes: getter implementations

* Net attributes getters changes: test fixes

* Remove net.sock.host.name

* code review comments

* default getter methods & getPeerSocketAddress() method name

* set authority in grpc earlier
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
…pen-telemetry#6503)

* Net attributes getters changes: instrumentation-api-semconv changes

* Net attributes getters changes: getter implementations

* Net attributes getters changes: test fixes

* Remove net.sock.host.name

* code review comments

* default getter methods & getPeerSocketAddress() method name

* set authority in grpc earlier
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
…pen-telemetry#6503)

* Net attributes getters changes: instrumentation-api-semconv changes

* Net attributes getters changes: getter implementations

* Net attributes getters changes: test fixes

* Remove net.sock.host.name

* code review comments

* default getter methods & getPeerSocketAddress() method name

* set authority in grpc earlier
@anuragagarwal561994
Copy link
Contributor

@mateuszrzeszutek this is an issue with the apache httpasyncclient instrumentation.

So we have used target.getAddress() here but the method was added in http core v4.3

If I see on maven central with apache httpasyncclient v4.1 https://mvnrepository.com/artifact/org.apache.httpcomponents/httpasyncclient/4.1

The default version that comes along for httpcore is 4.4.x but that doesn't mean that the user doesn't have a way to override this.

Hence someone using httpasyncclient 4.1.x with < 4.4.x version of httpcore, things will break.

@trask
Copy link
Member

trask commented Jan 30, 2023

@anuragagarwal561994 can you open a new issue for this comment so it does not get lost? thx!

@anuragagarwal561994
Copy link
Contributor

@trask done

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.

4 participants