Stats: Map deprecated RPC tags when record against new tags. #1854

songy23 · 2019-04-19T22:20:18Z

gRPC may migrate to use the new RPC tags (e.g "grpc_client_method") in grpc/grpc-java#5601 and stop recording against the deprecated tags (e.g "method"). However probably there're users who still use the deprecated RPC constants. To avoid breaking their metrics, we need to do some mappings when recording stats against the new tags.

Note the deprecated RPC measures are already mapped to the new ones in #1738.

Updates #1764.

zhangkun83 · 2019-04-19T22:52:53Z

impl_core/src/main/java/io/opencensus/implcore/stats/RecordUtils.java

+  // TODO(songy23): remove the mapping once we completely remove the deprecated RPC constants.
+  private static TagValue getTagValueForDeprecatedRpcTag(
+      Map<? extends TagKey, TagValueWithMetadata> tags, TagKey oldKey) {
+    for (TagKey newKey : RPC_TAG_MAPPINGS.get(oldKey)) {


@carl-mastrangelo, I think the for-each construct will create an Iterator, which you may not want to see. Maybe RPC_TAG_MAPPINGS should be defined as a Map<TagKey, TagKey[]> because for-each on an array presumably doesn't create an Iterator?

SGTM, updated.

dinooliva · 2019-04-19T22:54:11Z

impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java

+    Map<TagKey, TagValueWithMetadata> tags =
+        ImmutableMap.of(
+            RecordUtils.GRPC_CLIENT_METHOD, METHOD_V_WITH_MD,
+            RecordUtils.GRPC_CLIENT_STATUS, STATUS_V_WITH_MD);


What happens when we have both a client method and a server method?

Depending on what tag keys we have in the view:

If we have the new "client_method" or "server_method" key (or even both), everything works fine;

If we have the old "method" key, ~~a random value will be picked~~. While without this mapping the value will always be null.

IMO this should be less an issue because eventually users should migrate to use the new keys. WDYT?

By looking at getTagValueForDeprecatedRpcTag(), it seems the underlying data (columns?) is still in the old format, thus getTagValueForDeprecatedRpcTag() tries to get the values from the new tags in case the old tags are not set. If both "client_method" or "server_method" are set, "client_method" will be used. Is it going to be the case even after the old tags are removed?

So, iiuc, if we have a view with and old key (e.g. "rpc_method") and tags

contains tag rpc_method:v1 then v1 is used

otherwise, contains tag grpc_client_method:v2 then v2 is used

otherwise contains tag grpc_server_method:v3 then v3 is used

otherwise UNKNOWN_TAG_VALUE is used

Is that correct? If so, it may be worth enumerating in the unit tests. Also, when we have both grpc_client_method and grpc_server_method, which one matches closer to the original notion of rpc_method?

If both "client_method" or "server_method" are set, "client_method" will be used.

Apologies for the confusion, what Kun said is correct. If both are present and a view is using the old column, client method value will be picked.

if we have a view with and old key (e.g. "rpc_method") and tags

Yes the order is correct. I added a few tests on this.

If both "client_method" or "server_method" are set, "client_method" will be used. Is it going to be the case even after the old tags are removed?

No, the new "client_method" and "server_method" tags will be matched properly regardless of the old tag. This change is only trying to get the values from the new tags for old ~~tag~~ column if it's still in use. Added one more test in 6867966 to verify.

carl-mastrangelo · 2019-04-19T23:19:07Z

impl_core/src/main/java/io/opencensus/implcore/stats/RecordUtils.java

-        // replace not found key values by null.
-        tagValues.add(UNKNOWN_TAG_VALUE);
+        @javax.annotation.Nullable TagValue tagValue = UNKNOWN_TAG_VALUE;
+        if (RPC_TAG_MAPPINGS.containsKey(tagKey)) {


nit: this is a double lookup of the key. It would be better to just get() it, and check for Null

Done, good catch.

zhangkun83 · 2019-04-19T23:34:51Z

impl_core/src/main/java/io/opencensus/implcore/stats/RecordUtils.java

+    }
+    for (TagKey newKey : newKeys) {
+      if (tags.containsKey(newKey)) {
+        return tags.get(newKey).getTagValue();


nit: this is also a double lookup.

zhangkun83 · 2019-04-19T23:41:00Z

impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java

+    Map<TagKey, TagValueWithMetadata> tags =
+        ImmutableMap.of(
+            RecordUtils.GRPC_CLIENT_METHOD, METHOD_V_WITH_MD,
+            RecordUtils.GRPC_CLIENT_STATUS, STATUS_V_WITH_MD);


By looking at getTagValueForDeprecatedRpcTag(), it seems the underlying data (columns?) is still in the old format, thus getTagValueForDeprecatedRpcTag() tries to get the values from the new tags in case the old tags are not set. If both "client_method" or "server_method" are set, "client_method" will be used. Is it going to be the case even after the old tags are removed?

dinooliva · 2019-04-20T00:49:52Z

impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java

+            RecordUtils.GRPC_SERVER_METHOD, METHOD_V_WITH_MD,
+            RecordUtils.GRPC_SERVER_STATUS, STATUS_V_WITH_MD,
+            RecordUtils.GRPC_CLIENT_METHOD, METHOD_V_2_WITH_MD,
+            RecordUtils.GRPC_CLIENT_STATUS, STATUS_V_2_WITH_MD);


Yes - these tests clarify what's going on, thanks@ - you may want to add a test that contains GRPC_SERVER_METHOD, GRPC_CLIENT_METHOD, and RPC_METHOD to verify that the RPC_METHOD tag value does not unexpectedly get overridden by the other tags but I'll leave that up to you.

Good idea, added one in 9e371ba.

songy23 · 2019-04-20T01:45:51Z

Thanks for the review!

Fixes grpc#5593 and supersedes grpc#5601 Now that census-instrumentation/opencensus-java#1854 has been merged & released as 0.21.0. We can start using the method & status tags. Background: Opencensus introduced new tags for status and method (census-instrumentation/opencensus-java#1115). The old views that used those tags were deprecated and new views were created that used the new tags. However grpc-java wasn't updated to use the new tags due to concern of breaking existing metrics. This resulted in the old views being deprecated while the new views were broken (goomics grpc#50). census-instrumentation/opencensus-java#1854 added a compatibility layer to opencensus that would remap new tags to old tags for old views. This should unblock grpc to switching to the new tags while allowing old views to still be populated. That commit was released as part of opencensus 0.21, which grpc currently uses

Fixes #5593 and supersedes #5601 Now that census-instrumentation/opencensus-java#1854 has been merged & released as 0.21.0. We can start using the method & status tags. Background: Opencensus introduced new tags for status and method (census-instrumentation/opencensus-java#1115). The old views that used those tags were deprecated and new views were created that used the new tags. However grpc-java wasn't updated to use the new tags due to concern of breaking existing metrics. This resulted in the old views being deprecated while the new views were broken (goomics #50). census-instrumentation/opencensus-java#1854 added a compatibility layer to opencensus that would remap new tags to old tags for old views. This should unblock grpc to switching to the new tags while allowing old views to still be populated. That commit was released as part of opencensus 0.21, which grpc currently uses

songy23 added gRPC stats tags tech debt labels Apr 19, 2019

songy23 requested a review from zhangkun83 April 19, 2019 22:20

songy23 requested review from dinooliva, rghetia and a team as code owners April 19, 2019 22:20

googlebot added the cla: yes label Apr 19, 2019

songy23 mentioned this pull request Apr 19, 2019

core: Add new OpenCensus tags for method names grpc/grpc-java#5601

Closed

zhangkun83 reviewed Apr 19, 2019

View reviewed changes

dinooliva reviewed Apr 19, 2019

View reviewed changes

carl-mastrangelo reviewed Apr 19, 2019

View reviewed changes

zhangkun83 reviewed Apr 19, 2019

View reviewed changes

songy23 force-pushed the map-grpc-method-tag branch 2 times, most recently from 72957b7 to cec1f5a Compare April 20, 2019 00:43

dinooliva approved these changes Apr 20, 2019

View reviewed changes

songy23 added 6 commits April 19, 2019 17:53

Stats: Map deprecated RPC tags when record against new tags.

4ae04e2

Fix review comments. Fix nullness checker.

0af2ae6

Avoid double looking up.

a1ecb48

Add more tests.

d7520d9

Add one more test on new tags.

6867966

Add a test that also sets the old tag.

9e371ba

songy23 force-pushed the map-grpc-method-tag branch from f859b05 to 9e371ba Compare April 20, 2019 00:53

zhangkun83 approved these changes Apr 20, 2019

View reviewed changes

songy23 merged commit c35bc41 into census-instrumentation:master Apr 20, 2019

songy23 deleted the map-grpc-method-tag branch April 20, 2019 01:46

igorbernstein2 mentioned this pull request Jul 21, 2019

core: Migrate to new OpenCensus method & status tags grpc/grpc-java#5996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stats: Map deprecated RPC tags when record against new tags. #1854

Stats: Map deprecated RPC tags when record against new tags. #1854

songy23 commented Apr 19, 2019 •

edited

zhangkun83 Apr 19, 2019

songy23 Apr 19, 2019

dinooliva Apr 19, 2019

songy23 Apr 19, 2019 •

edited

zhangkun83 Apr 19, 2019

dinooliva Apr 19, 2019

songy23 Apr 20, 2019

songy23 Apr 20, 2019 •

edited

carl-mastrangelo Apr 19, 2019

songy23 Apr 20, 2019

zhangkun83 Apr 19, 2019

songy23 Apr 20, 2019

zhangkun83 Apr 19, 2019

dinooliva Apr 20, 2019

songy23 Apr 20, 2019

songy23 commented Apr 20, 2019

Stats: Map deprecated RPC tags when record against new tags. #1854

Stats: Map deprecated RPC tags when record against new tags. #1854

Conversation

songy23 commented Apr 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 Apr 19, 2019 • edited

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

songy23 Apr 20, 2019 • edited

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

songy23 commented Apr 20, 2019

songy23 commented Apr 19, 2019 •

edited

songy23 Apr 19, 2019 •

edited

songy23 Apr 20, 2019 •

edited