Switch from http.flavor to net.protocol.* in HTTP client instrumentat… #8131

mateuszrzeszutek · 2023-03-24T15:19:05Z

…ions

Implements half of the open-telemetry/opentelemetry-specification#3272 spec PR; and half of the #4519 issue.

trask · 2023-03-28T19:29:17Z

...etry/instrumentation/api/instrumenter/net/internal/InternalNetClientAttributesExtractor.java

+ String protocolName = getter.getProtocolName(request, response);
+ if (protocolName != null) {
+ internalSet(
+ attributes, NetAttributes.NET_PROTOCOL_NAME, protocolName.toLowerCase(Locale.ROOT));


what do you think of moving toLowerCase into the getProtocolName implementations that need it?

That was my initial idea, but I've decided to have the conversion here because of how this attribute is described in the spec:

Application layer protocol used. The value SHOULD be normalized to lowercase.

I'm thinking that any normalization should probably be done in the extractor; there's a similarity to the SQL sanitization, which also happens in the extractor.

Application layer protocol used. The value SHOULD be normalized to lowercase.

I think of the semantic convention definitions as applying to the getter methods, and I think it could become slightly more confusing if some parts of the semantic convention are handled in the extractor and some parts are handled in the getter methods.

I think the existing SQL sanitization is a bit ok, because in that case the getter method is getRawStatement() as opposed to getStatement()

I think of the semantic convention definitions as applying to the getter methods, and I think it could become slightly more confusing if some parts of the semantic convention are handled in the extractor and some parts are handled in the getter methods.

That's a good point.
My idea about the extractors and semantic conventions was: put as much as possible into extractors, and make implementing the getters as easy as possible - someone who uses the instrumentation api to instrument their own library should not have to know the intricacies of our semantic conventions, the extractors are kinda "smart" and can massage the data a bit. We already do things like that with e.g. removing auth info from http.url, or the conditional on the net.sock.family (skip if default inet).

...ain/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientRequest.java

...ntelemetry/javaagent/instrumentation/apachehttpclient/v5_0/AbstractApacheHttpClientTest.java

...metry/javaagent/instrumentation/asynchttpclient/v2_0/AsyncHttpClientNetAttributesGetter.java

trask · 2023-03-28T19:44:11Z

...io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlNetAttributesGetter.java

+ @Override
+ public String getProtocolName(HttpURLConnection connection, @Nullable Integer integer) {
+ // HttpURLConnection hardcodes the protocol name&version
+ return "http";


should we not capture since this is the default? cc @lmolkova

yep, great catch!

Just to clarify: should we not capture http at all; not capture http 1.1; or something else?

my reading is that we should capture the version (1.1) if we know it, but not the protocol

Hm, why? The net.protocol.name attribute is (or will be) shared across different semantic conventions, it's not like it has a constant default value that we can just skip (like net.sock.family has inet). IMO knowing that version is 1.1 without knowing the name of protocol sends a somewhat unclear message - you can guess that it actually is http by looking at the other attributes though.

Interesting. So we don't have a good way to enforce defaults for net attributes on higher-level instrumentations, right?
I.e. populating net.protocol.version = http on HTTP spans is pure redundancy, but then we'd have to have http-specific implementation of net attribute extractor

I.e. populating net.protocol.version = http on HTTP spans is pure redundancy, but then we'd have to have http-specific implementation of net attribute extractor

I'm actually thinking about having the HTTP getter extend the net getter (just as http semconv kind of "extends" the net semconv); but even in that case I wouldn't say that it's 100% redundant. There are some HTTP clients that support QUIC or SPDY - and while I think that almost nobody actually uses these protocols, there is still a possibility to use them, and the instrumentation should be prepared to reflect that.

trask · 2023-03-28T19:49:18Z

...in/java/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesGetter.java

- default String getProtocolVersion(REQUEST request, @Nullable RESPONSE response) {
- return null;
- }
+ String getProtocolVersion(REQUEST request, @Nullable RESPONSE response);


@lmolkova do you have any thought how common the protocol name/version will be, and whether we should have default methods for these (similar to the sock attributes below), or whether it's good to force implementers to think about / implement these?

BTW sort of related: I've been thinking of making getTransport() optional, since there are few instrumentations that actually know the transport protocol (instead of just hardcoding TCP_IP and hoping it's correct)

it makes sense for protocol information to be left unset by default - it does not seem that essential and instrumentations that care can override.

wrt transport - I think we removed it from HTTP spec completely. HTTP, gRPC, and most of the other protocols would always have TCP there, maybe we should just add a default value to the spec?

HTTP/3 uses UDP. gRPC offers an in-process transport for which the hardcoded IP_TCP is probably wrong.

I'll make the transport optional in a separate PR -- and the protocol attributes in this PR, since it kinda reverts some of my changes.

HTTP/3 uses UDP. gRPC offers an in-process transport for which the hardcoded IP_TCP is probably wrong.

Agree, I think the default on the instrumentation side should be null. Spec can have language (for the backends) to assume transport from protocol name/version. E.g. HTTP/3 is UDP, there is no need to duplicate it on telemetry.

For less popular protocols, it might make sense to set both protocol name/version AND transport, but their specs can require transport if needed.

I applied [this comment](#8131 (comment)) to the whole codebase and removed some `Optional`s that were used in the hot path

Based on #8190 and #8131 (comment) --------- Co-authored-by: Mateusz Rzeszutek <[email protected]>

trask

@mateuszrzeszutek thanks! can you open an issue(s) for potentially open questions?

let's merge if you think you have time for server side before the release next week, otherwise let's hold this PR until after the release

…ions

mateuszrzeszutek requested a review from a team as a code owner March 24, 2023 15:19

mateuszrzeszutek force-pushed the net.protocol-client branch 2 times, most recently from dc91435 to 84f2c85 Compare March 27, 2023 06:47

trask reviewed Mar 28, 2023

View reviewed changes

mateuszrzeszutek mentioned this pull request Mar 31, 2023

Remove some Optional usages #8190

Merged

trask mentioned this pull request Mar 31, 2023

Add note about Optional/Stream usage to style guide #8193

Merged

mateuszrzeszutek pushed a commit that referenced this pull request Apr 3, 2023

Remove some Optional usages (#8190)

46e5219

I applied [this comment](#8131 (comment)) to the whole codebase and removed some `Optional`s that were used in the hot path

trask added a commit that referenced this pull request Apr 3, 2023

Add note about Optional/Stream usage to style guide (#8193)

3e087e8

Based on #8190 and #8131 (comment) --------- Co-authored-by: Mateusz Rzeszutek <[email protected]>

mateuszrzeszutek force-pushed the net.protocol-client branch from fcb95a5 to 0f28ccc Compare April 4, 2023 14:38

trask approved these changes Apr 5, 2023

View reviewed changes

Mateusz Rzeszutek added 13 commits April 6, 2023 18:14

Switch from http.flavor to net.protocol.* in HTTP client instrumentat…

a682618

…ions

r2dbc

6b1777a

fix jodd test compilation

cdb4a57

fix tests

5df5d12

fix tests

0573b66

fix tests

2db9a9a

fix tests

b63a507

fix tests

d676f19

codenarc

340087f

fix tests

546135c

code review comments

9280672

errorprone

9ef8b0a

Add defaults back to getProtocol*()

d63d44e

mateuszrzeszutek force-pushed the net.protocol-client branch from 0f28ccc to d63d44e Compare April 6, 2023 16:14

mateuszrzeszutek merged commit f501569 into open-telemetry:main Apr 7, 2023

mateuszrzeszutek deleted the net.protocol-client branch April 7, 2023 11:39

This was referenced Apr 7, 2023

Switch from http.flavor to net.protocol.* in HTTP server instrumentat… #8244

Merged

Make net.transport an optional attribute #8245

Closed

Getters vs extractors: where should we draw the line? #8246

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from http.flavor to net.protocol.* in HTTP client instrumentat… #8131

Switch from http.flavor to net.protocol.* in HTTP client instrumentat… #8131

mateuszrzeszutek commented Mar 24, 2023

trask Mar 28, 2023

mateuszrzeszutek Mar 29, 2023

trask Apr 2, 2023

mateuszrzeszutek Apr 4, 2023

trask Mar 28, 2023

lmolkova Apr 4, 2023

mateuszrzeszutek Apr 4, 2023

trask Apr 4, 2023

mateuszrzeszutek Apr 4, 2023

lmolkova Apr 4, 2023

mateuszrzeszutek Apr 5, 2023

trask Mar 28, 2023

mateuszrzeszutek Mar 29, 2023

lmolkova Apr 4, 2023

laurit Apr 4, 2023

mateuszrzeszutek Apr 4, 2023

lmolkova Apr 4, 2023

trask left a comment

Switch from http.flavor to net.protocol.* in HTTP client instrumentat… #8131

Switch from http.flavor to net.protocol.* in HTTP client instrumentat… #8131

Conversation

mateuszrzeszutek commented Mar 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment