Add new RPC measure and view constants, deprecate old ones. by songy23 · Pull Request #1115 · census-instrumentation/opencensus-java

songy23 · 2018-04-06T01:11:53Z

Update RPC constants according to specs and our meeting today.

Equivalent changes to OC C++: census-instrumentation/opencensus-cpp#138.

codecov-io · 2018-04-06T01:26:06Z

Codecov Report

Merging #1115 into master will increase coverage by 0.28%.
The diff coverage is 98.07%.

@@             Coverage Diff             @@
##             master   #1115      +/-   ##
===========================================
+ Coverage     81.52%   81.8%   +0.28%     
- Complexity     1203    1205       +2     
===========================================
  Files           185     185              
  Lines          5617    5716      +99     
  Branches        523     523              
===========================================
+ Hits           4579    4676      +97     
- Misses          894     896       +2     
  Partials        144     144

Impacted Files	Coverage Δ	Complexity Δ
...ncensus/contrib/grpc/metrics/RpcViewConstants.java	`99.72% <100%> (+0.05%)`	`1 <0> (ø)`	⬇️
...nsus/contrib/grpc/metrics/RpcMeasureConstants.java	`98.7% <100%> (+0.82%)`	`1 <1> (ø)`	⬇️
...a/io/opencensus/contrib/grpc/metrics/RpcViews.java	`70% <75%> (+1.81%)`	`8 <2> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f852100...c428fe8. Read the comment docs.

bogdandrutu · 2018-04-06T01:57:51Z

+   *
+   * @since 0.13
+   */
+  public static final TagKey RPC_CLIENT_STATUS = TagKey.create("grpc_client_status");


Why not GRPC_CLIENT_STATUS?

Done. I updated all the new constants with prefix "GRPC_".

bogdandrutu · 2018-04-06T01:58:19Z

+   *
+   * @since 0.13
+   */
+  public static final MeasureDouble RPC_CLIENT_SENT_BYTES_PER_RPC =


Same here. Why not GRPC_CLIENT_SENT_BYTES_PER_RPC?

bogdandrutu · 2018-04-06T02:02:44Z

          RpcViewConstants.RPC_SERVER_UNCOMPRESSED_RESPONSE_BYTES_VIEW,
          RpcViewConstants.RPC_SERVER_STARTED_COUNT_CUMULATIVE_VIEW,
-          RpcViewConstants.RPC_SERVER_FINISHED_COUNT_CUMULATIVE_VIEW);
+          RpcViewConstants.RPC_SERVER_FINISHED_COUNT_CUMULATIVE_VIEW,


I would not put the new views in the same list, instead I will create a new list and have a different method that installs all the new views.

Then mark the old one as deprecated and suggest to use the new one, also explaining what it will happen (user may no longer have data in the old metrics) and suggesting that user may decide to still register the old views while they will deprecate the old views.

Sounds good, I added a new method registerAllGrpcViews that registers all the new views. While registerAllViews will register both new and old views.

I think the old one should register just the old views.

Done, though the method name registerAllViews could be a little confusing while it only registers the deprecated views.

songy23 · 2018-04-06T19:31:48Z

Uncompressed bytes and finished count are no longer needed, though we still need the started count measure.

songy23 · 2018-04-09T23:59:39Z

Thanks!

sebright2

I reviewed everything but the View constants.

sebright2 · 2018-04-06T18:24:23Z

+
+  /**
+   * Tag key that represents a client gRPC canonical status. Refer to
+   * https://github.com/grpc/grpc/blob/master/doc/statuscodes.md.


It would help to explain the difference between the client and server status tags. When should each of them be set? The same applies to the client and server method tags.

Good question but I don't have a clear answer for it. I created census-instrumentation/opencensus-specs#95 to follow up.

sebright2 · 2018-04-06T18:52:20Z

   * @since 0.8
   */
+  @Deprecated
  public static final MeasureLong RPC_CLIENT_REQUEST_COUNT =


Is this replaced by GRPC_CLIENT_SENT_MESSAGES_PER_RPC?

Good catch, I missed this one.

sebright2 · 2018-04-06T18:52:52Z

   * @since 0.8
   */
+  @Deprecated
  public static final MeasureLong RPC_CLIENT_RESPONSE_COUNT =


Is this replaced by GRPC_CLIENT_RECEIVED_MESSAGES_PER_RPC?

sebright2 · 2018-04-09T23:49:57Z

+   */
+  public static final MeasureLong GRPC_CLIENT_STARTED_COUNT =
+      Measure.MeasureLong.create(
+          "grpc.io/client/started_count", "Number of client RPCs (streams) started", COUNT);


Should this measure be added to the spec? I couldn't find it.

I noted that Go has removed started count in census-instrumentation/opencensus-go#689. C++ doesn't have started count either. I'll remove the started count measures and views.

sebright2 · 2018-04-09T23:54:19Z

   * {@link Measure} for gRPC server request bytes.
   *
   * @since 0.8
+   * @deprecated in favor of {@link #GRPC_SERVER_SENT_BYTES_PER_RPC}.


I think that GRPC_SERVER_SENT_BYTES_PER_RPC and GRPC_SERVER_RECEIVED_BYTES_PER_RPC are swapped here and in the next Javadoc.

Good catch, I just noticed the direction for server request/response should be the reverse to client. Updated.

sebright2 · 2018-04-10T00:10:38Z

+   */
+  public static final MeasureLong GRPC_SERVER_STARTED_COUNT =
+      Measure.MeasureLong.create(
+          "grpc.io/server/started_count", "Number of server RPCs (streams) started", COUNT);


I also couldn't find this measure in the spec.

sebright2 · 2018-04-10T00:13:10Z

+   */
+  public static final MeasureLong GRPC_SERVER_SENT_MESSAGES_PER_RPC =
+      Measure.MeasureLong.create(
+          "grpc.io/server/sent_messages_per_rpc", "Number of messages sent in the RPC", COUNT);


sebright2 · 2018-04-10T00:13:57Z

+  public static final MeasureLong GRPC_SERVER_RECEIVED_MESSAGES_PER_RPC =
+      Measure.MeasureLong.create(
+          "grpc.io/server/received_messages_per_rpc",
+          "Number of response messages received per RPC",


s/response/request

I removed the word "response" to be consistent with specs:

grpc.io/server/received_messages_per_rpc: Number of messages received in each RPC.

sebright2 · 2018-04-10T00:21:29Z

+              0.0, 0.01, 0.05, 0.1, 0.3, 0.6, 0.8, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 8.0, 10.0, 13.0,
+              16.0, 20.0, 25.0, 30.0, 40.0, 50.0, 65.0, 80.0, 100.0, 130.0, 160.0, 200.0, 250.0,
+              300.0, 400.0, 500.0, 650.0, 800.0, 1000.0, 2000.0, 5000.0, 10000.0, 20000.0, 50000.0,
+              100000.0));


Does this change any existing views? I think the deprecated views should continue using the original histogram buckets.

Make sense, I updated the deprecated views to continue using the old buckets (though I think it's unlikely to break existing views unless users are reading the buckets from views).

sebright2 · 2018-04-10T00:24:35Z

+   * @since 0.13
+   */
+  public static void registerAllGrpcViews() {
+    registerAllCumulativeViews(Stats.getViewManager());


Shouldn't this call registerAllGrpcViews(ViewManager)?

That's right, done.

songy23

Posted #1118 to fix the issues.

songy23 · 2018-04-10T00:38:50Z

   * @since 0.8
   */
+  @Deprecated
  public static final MeasureLong RPC_CLIENT_REQUEST_COUNT =


Good catch, I missed this one.

songy23 · 2018-04-10T00:39:02Z

   * @since 0.8
   */
+  @Deprecated
  public static final MeasureLong RPC_CLIENT_RESPONSE_COUNT =


songy23 · 2018-04-10T00:40:17Z

+   */
+  public static final MeasureLong GRPC_CLIENT_STARTED_COUNT =
+      Measure.MeasureLong.create(
+          "grpc.io/client/started_count", "Number of client RPCs (streams) started", COUNT);


I noted that Go has removed started count in census-instrumentation/opencensus-go#689. C++ doesn't have started count either. I'll remove the started count measures and views.

songy23 · 2018-04-10T00:44:27Z

   * {@link Measure} for gRPC server request bytes.
   *
   * @since 0.8
+   * @deprecated in favor of {@link #GRPC_SERVER_SENT_BYTES_PER_RPC}.


Good catch, I just noticed the direction for server request/response should be the reverse to client. Updated.

songy23 · 2018-04-10T00:44:33Z

   * {@link Measure} for gRPC server uncompressed request bytes.
   *
   * @since 0.8
+   * @deprecated in favor of {@link #GRPC_SERVER_SENT_BYTES_PER_RPC}.


songy23 · 2018-04-10T00:51:15Z

+  public static final MeasureLong GRPC_SERVER_RECEIVED_MESSAGES_PER_RPC =
+      Measure.MeasureLong.create(
+          "grpc.io/server/received_messages_per_rpc",
+          "Number of response messages received per RPC",


I removed the word "response" to be consistent with specs:

grpc.io/server/received_messages_per_rpc: Number of messages received in each RPC.

songy23 · 2018-04-10T00:59:40Z

+
+  /**
+   * Tag key that represents a client gRPC canonical status. Refer to
+   * https://github.com/grpc/grpc/blob/master/doc/statuscodes.md.


Good question but I don't have a clear answer for it. I created census-instrumentation/opencensus-specs#95 to follow up.

songy23 · 2018-04-10T01:04:23Z

+   */
+  public static final MeasureLong GRPC_SERVER_STARTED_COUNT =
+      Measure.MeasureLong.create(
+          "grpc.io/server/started_count", "Number of server RPCs (streams) started", COUNT);


songy23 · 2018-04-10T01:07:12Z

+              0.0, 0.01, 0.05, 0.1, 0.3, 0.6, 0.8, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 8.0, 10.0, 13.0,
+              16.0, 20.0, 25.0, 30.0, 40.0, 50.0, 65.0, 80.0, 100.0, 130.0, 160.0, 200.0, 250.0,
+              300.0, 400.0, 500.0, 650.0, 800.0, 1000.0, 2000.0, 5000.0, 10000.0, 20000.0, 50000.0,
+              100000.0));


Make sense, I updated the deprecated views to continue using the old buckets (though I think it's unlikely to break existing views unless users are reading the buckets from views).

songy23 · 2018-04-10T01:08:12Z

+   * @since 0.13
+   */
+  public static void registerAllGrpcViews() {
+    registerAllCumulativeViews(Stats.getViewManager());


That's right, done.

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 labels Apr 6, 2018

songy23 added this to the Release 0.13.0 milestone Apr 6, 2018

songy23 assigned bogdandrutu, dinooliva and sebright2 Apr 6, 2018

songy23 requested review from bogdandrutu, dinooliva and sebright2 April 6, 2018 01:11

googlebot added the cla: yes label Apr 6, 2018

bogdandrutu reviewed Apr 6, 2018

View reviewed changes

songy23 assigned songy23 and unassigned bogdandrutu Apr 6, 2018

songy23 added 3 commits April 6, 2018 10:27

Add new RPC measures according to specs, deprecate old ones.

9467b69

Add new RPC views according to specs, deprecate old ones.

985ee2b

Registers new gRPC views with a new method.

1ca1952

songy23 force-pushed the rpc-constants branch from d3af181 to 1ca1952 Compare April 6, 2018 17:31

songy23 assigned bogdandrutu and unassigned songy23 Apr 6, 2018

songy23 added 3 commits April 6, 2018 10:40

Fix javadoc link

1b16c37

Remove uncompressed bytes and finished count.

cde2f81

Do not register new gRPC views for registerAllViews.

8ddc9c4

Fix javadoc links.

c428fe8

songy23 mentioned this pull request Apr 6, 2018

Update gRPC client and server metrics based on Specs. census-instrumentation/opencensus-go#687

Closed

bogdandrutu approved these changes Apr 9, 2018

View reviewed changes

songy23 merged commit bb53c02 into census-instrumentation:master Apr 9, 2018

sebright2 reviewed Apr 10, 2018

View reviewed changes

songy23 mentioned this pull request Apr 10, 2018

Fix issues in RPC constants #1118

Merged

songy23 commented Apr 10, 2018

View reviewed changes

songy23 mentioned this pull request May 22, 2018

Update gRPC to use the new RPC constants, and record against the new ones #1207

Closed

igorbernstein2 mentioned this pull request Jul 21, 2019

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

Merged

Conversation

songy23 commented Apr 6, 2018

Uh oh!

codecov-io commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songy23 commented Apr 6, 2018

Uh oh!

songy23 commented Apr 9, 2018

Uh oh!

sebright2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songy23 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-io commented Apr 6, 2018 •

edited

Loading