add metrics rpc conventions implement #4838

yangtaoran · 2021-12-08T06:38:08Z

No description provided.

yangtaoran · 2021-12-08T06:42:18Z

hi @trask,the EasyCLA check have passed,thank you.

mateuszrzeszutek

LGTM 👍
Thanks! I've left a couple of comments here, but they're all fairly minor things.

...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcClientMetrics.java

...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcServerMetrics.java

yangtaoran · 2021-12-08T11:53:16Z

Thanks for @mateuszrzeszutek suggestions,and I have resolve them.

trask

thx @yangtaoran!

can you add tests similar to HttpClientMetricsTest and HttpServerMetricsTest?

trask · 2021-12-08T21:55:46Z

...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/MetricsView.java

+    // the list of optional metrics attributes is from
+    // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/rpc.md#attributes
+    Set<AttributeKey> view = new HashSet<>();
+    view.add(SemanticAttributes.NET_PEER_IP);
+    view.add(SemanticAttributes.NET_PEER_NAME);
+    view.add(SemanticAttributes.NET_PEER_PORT);
+    view.add(SemanticAttributes.NET_TRANSPORT);
+    return view;


cc @jsuereth: this follows the spec, but I'm worried about the cardinality issue you raised previously, wdyt?

According to the linked doc:

To avoid high cardinality, implementations should prefer the most stable of net.peer.name or net.peer.ip, depending on expected deployment profile. For many cloud applications, this is likely net.peer.name as names can be recycled even across re-instantiation of a server with a different ip.

I.e. you can fallback to net.peer.ip if it's you're only option, but you should JUST use net.peer.name if you can.

@jsuereth so we should go ahead and capture these same attributes on server metrics (unlike on http server metrics)? I guess the situation is not as bad in RPC-land as in browser-land (except for maybe android/grpc?)

We may want to re-evaluate server side using a notion of endpoint vs. net.peer.ip for similar reasons to http. However the specification is based on what gRPC does. I'll do some homework here to see if this is still how that's instrumented.

Key thing though, we should be using hostname over IP because hostname should be more stable, especially in the presence of a load balancer.

I understanding is usering net.peer.name instead of net.peer.ip if the attribute net.peer.name exist.

Yeah, the other issue here is around whether we should be using net.peer.* on RPC server-metrics.

We translated the specification on instrumentation from OpenCensus gRPC that lead to this as a basis. HOWEVER, there's a few caveats:

In OpenCensus instrumentation + Views were separately specified. Instrumentation was thorough, views were more conservative. When this was originally written for OTEL the View spec wasn't known, so that part is unspecified. In OpenCensus, views are where the high cardinality issues were dealt with by use case, and where you could select styles of latency metrics to export (e.g. if you wanted SLO monitoring). I think the balance we struck 13 months ago is likely now wrong given the evolution of our APIs/SDKs unless we provide out-of-the-box VIew configuration for these metrics in addition to the rich instrumentation attributes.

RPC semantic conventions now diverge from HTTP semantic conventions. Effectively.... "client" metrics use net.peer.name and "host" metrics should use host.name in HTTP, but RPC both use net.peer.

For both of these reasons, we may want to re-evaluate RPC metrics. Specifically RPC server metrics.

Opened open-telemetry/opentelemetry-specification#2214

So I can modify the RPC conventions implement like this:
Client metrics use net.peer.name (or if unavailable net.peer.ip),net.peer.port,net.transport.
Server metrics use net.host.name (or if unavailable nethost.ip),net.transport,exclude net.host.port.
Do you think this is ok?

That's effectively what I'd do.

For the purpose of this PR, I think if you just don't fill out any net.* attributes on the server side you're fine (because of Resource attributes). As we discussed in the Java-SiG yesterday, it'd be good to mark this instrumentation as "experimental" in some fashion so you can try a few options out and we can see how they work in practice.

...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcClientMetrics.java

...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcServerMetrics.java

yangtaoran · 2021-12-09T08:51:22Z

thx @yangtaoran!

can you add tests similar to HttpClientMetricsTest and HttpServerMetricsTest?

hi @trask I have added RpcClientMetricsTest and RpcServerMetricsTest similar to HttpClientMetricsTest and HttpServerMetricsTest

mateuszrzeszutek · 2021-12-09T15:23:05Z

...rc/test/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcClientMetricsTest.java

+    // TODO(anuraaga): Remove await from this file after 1.8.0 hopefully fixes
+    // https://github.com/open-telemetry/opentelemetry-java/issues/3725


@anuraaga I tried removing awaits from HttpClientMetricsTest/HttpServerMetricsTest and it failed for me; looks like this issue is still not solved.

mateuszrzeszutek · 2021-12-09T15:27:54Z

...rc/test/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcClientMetricsTest.java

+            .put(SemanticAttributes.NET_TRANSPORT, "ip_tcp")
+            .build();
+
+    Attributes responseAttributes2 =


WDYT about splitting this into 2 test cases? One for host name, one for IP.

Yeah,according to @jsuereth suggestion,we should pefer to net.peer.name, we can fallback to net.peer.ip if net.peer.name is unavailable.

trask

this looks great, thx!

...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcClientMetrics.java

...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcServerMetrics.java

…tion/api/instrumenter/rpc/RpcServerMetrics.java

…tion/api/instrumenter/rpc/RpcClientMetrics.java

trask · 2021-12-11T00:30:11Z

Thanks again @yangtaoran!

* add metrics rpc conventions * handle format violations * handle format violations * resolve the comments suggestion * update RpcClientMetrics format * update RpcServerMetrics format * resolve time precision and cardinality issue * invoke buildServerFallbackView method * add RpcServerMetricsTest and RpcClientMetricsTest * server metrics attibutes remove net.perr* and add net.host * Update instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcServerMetrics.java * Update instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcClientMetrics.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

add metrics rpc conventions

da1113c

yangtaoran requested review from anuraaga, iNikem, jkwatson, laurit, mateuszrzeszutek, pavolloffay, trask and tylerbenson as code owners December 8, 2021 06:38

handle format violations

92e4009

mateuszrzeszutek approved these changes Dec 8, 2021

View reviewed changes

yangtaoran added 2 commits December 8, 2021 19:24

handle format violations

7017744

resolve the comments suggestion

f1fb64c

yangtaoran added 2 commits December 8, 2021 22:04

update RpcClientMetrics format

4cb8305

update RpcServerMetrics format

52c2004

trask reviewed Dec 8, 2021

View reviewed changes

yangtaoran added 3 commits December 9, 2021 14:37

resolve time precision and cardinality issue

993cc12

invoke buildServerFallbackView method

411507e

add RpcServerMetricsTest and RpcClientMetricsTest

53765cb

mateuszrzeszutek reviewed Dec 9, 2021

View reviewed changes

trask approved these changes Dec 9, 2021

View reviewed changes

...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcClientMetrics.java Outdated Show resolved Hide resolved

...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/rpc/RpcServerMetrics.java Outdated Show resolved Hide resolved

yangtaoran and others added 3 commits December 10, 2021 17:43

server metrics attibutes remove net.perr* and add net.host

393d6f9

Update instrumentation-api/src/main/java/io/opentelemetry/instrumenta…

833144f

…tion/api/instrumenter/rpc/RpcServerMetrics.java

Update instrumentation-api/src/main/java/io/opentelemetry/instrumenta…

e25565e

…tion/api/instrumenter/rpc/RpcClientMetrics.java

trask merged commit c59bebf into open-telemetry:main Dec 11, 2021

yangtaoran mentioned this pull request Dec 11, 2021

When implement RPC metrics #4747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add metrics rpc conventions implement #4838

add metrics rpc conventions implement #4838

yangtaoran commented Dec 8, 2021

yangtaoran commented Dec 8, 2021

mateuszrzeszutek left a comment

yangtaoran commented Dec 8, 2021

trask left a comment

trask Dec 8, 2021

jsuereth Dec 8, 2021

trask Dec 8, 2021

jsuereth Dec 9, 2021

yangtaoran Dec 9, 2021

jsuereth Dec 9, 2021

jsuereth Dec 9, 2021

yangtaoran Dec 10, 2021

jsuereth Dec 10, 2021

yangtaoran commented Dec 9, 2021 •

edited by trask

Loading

mateuszrzeszutek Dec 9, 2021

mateuszrzeszutek Dec 9, 2021

yangtaoran Dec 10, 2021

trask left a comment

trask commented Dec 11, 2021

		// TODO(anuraaga): Remove await from this file after 1.8.0 hopefully fixes
		// https://github.com/open-telemetry/opentelemetry-java/issues/3725

add metrics rpc conventions implement #4838

add metrics rpc conventions implement #4838

Conversation

yangtaoran commented Dec 8, 2021

yangtaoran commented Dec 8, 2021

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

yangtaoran commented Dec 8, 2021

trask left a 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

yangtaoran commented Dec 9, 2021 • edited by trask Loading

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

trask commented Dec 11, 2021

yangtaoran commented Dec 9, 2021 •

edited by trask

Loading