-
Notifications
You must be signed in to change notification settings - Fork 202
Add new RPC measure and view constants, deprecate old ones. #1115
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
* | ||
* @since 0.13 | ||
*/ | ||
public static final TagKey RPC_CLIENT_STATUS = TagKey.create("grpc_client_status"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not GRPC_CLIENT_STATUS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I updated all the new constants with prefix "GRPC_".
* | ||
* @since 0.13 | ||
*/ | ||
public static final MeasureDouble RPC_CLIENT_SENT_BYTES_PER_RPC = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Why not GRPC_CLIENT_SENT_BYTES_PER_RPC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -52,7 +53,26 @@ | |||
RpcViewConstants.RPC_SERVER_UNCOMPRESSED_REQUEST_BYTES_VIEW, | |||
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I added a new method registerAllGrpcViews
that registers all the new views. While registerAllViews
will register both new and old views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the old one should register just the old views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, though the method name registerAllViews could be a little confusing while it only registers the deprecated views.
Uncompressed bytes and finished count are no longer needed, though we still need the started count measure. |
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed everything but the View constants.
|
||
/** | ||
* Tag key that represents a client gRPC canonical status. Refer to | ||
* https://github.com/grpc/grpc/blob/master/doc/statuscodes.md. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question but I don't have a clear answer for it. I created census-instrumentation/opencensus-specs#95 to follow up.
@@ -151,6 +202,7 @@ | |||
* | |||
* @since 0.8 | |||
*/ | |||
@Deprecated | |||
public static final MeasureLong RPC_CLIENT_REQUEST_COUNT = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this replaced by GRPC_CLIENT_SENT_MESSAGES_PER_RPC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I missed this one.
@@ -160,41 +212,154 @@ | |||
* | |||
* @since 0.8 | |||
*/ | |||
@Deprecated | |||
public static final MeasureLong RPC_CLIENT_RESPONSE_COUNT = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this replaced by GRPC_CLIENT_RECEIVED_MESSAGES_PER_RPC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
public static final MeasureLong GRPC_CLIENT_STARTED_COUNT = | ||
Measure.MeasureLong.create( | ||
"grpc.io/client/started_count", "Number of client RPCs (streams) started", COUNT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this measure be added to the spec? I couldn't find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
public static final MeasureLong RPC_SERVER_ERROR_COUNT = | ||
Measure.MeasureLong.create("grpc.io/server/error_count", "RPC Errors", COUNT); | ||
|
||
/** | ||
* {@link Measure} for gRPC server request bytes. | ||
* | ||
* @since 0.8 | ||
* @deprecated in favor of {@link #GRPC_SERVER_SENT_BYTES_PER_RPC}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that GRPC_SERVER_SENT_BYTES_PER_RPC and GRPC_SERVER_RECEIVED_BYTES_PER_RPC are swapped here and in the next Javadoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I just noticed the direction for server request/response should be the reverse to client. Updated.
*/ | ||
public static final MeasureLong GRPC_SERVER_STARTED_COUNT = | ||
Measure.MeasureLong.create( | ||
"grpc.io/server/started_count", "Number of server RPCs (streams) started", COUNT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also couldn't find this measure in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
*/ | ||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/the/each
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/response/request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the word "response" to be consistent with specs:
grpc.io/server/received_messages_per_rpc: Number of messages received in each RPC.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change any existing views? I think the deprecated views should continue using the original histogram buckets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
* @since 0.13 | ||
*/ | ||
public static void registerAllGrpcViews() { | ||
registerAllCumulativeViews(Stats.getViewManager()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this call registerAllGrpcViews(ViewManager)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted #1118 to fix the issues.
@@ -151,6 +202,7 @@ | |||
* | |||
* @since 0.8 | |||
*/ | |||
@Deprecated | |||
public static final MeasureLong RPC_CLIENT_REQUEST_COUNT = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I missed this one.
@@ -160,41 +212,154 @@ | |||
* | |||
* @since 0.8 | |||
*/ | |||
@Deprecated | |||
public static final MeasureLong RPC_CLIENT_RESPONSE_COUNT = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
public static final MeasureLong GRPC_CLIENT_STARTED_COUNT = | ||
Measure.MeasureLong.create( | ||
"grpc.io/client/started_count", "Number of client RPCs (streams) started", COUNT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
public static final MeasureLong RPC_SERVER_ERROR_COUNT = | ||
Measure.MeasureLong.create("grpc.io/server/error_count", "RPC Errors", COUNT); | ||
|
||
/** | ||
* {@link Measure} for gRPC server request bytes. | ||
* | ||
* @since 0.8 | ||
* @deprecated in favor of {@link #GRPC_SERVER_SENT_BYTES_PER_RPC}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I just noticed the direction for server request/response should be the reverse to client. Updated.
@@ -212,7 +349,9 @@ | |||
* {@link Measure} for gRPC server uncompressed request bytes. | |||
* | |||
* @since 0.8 | |||
* @deprecated in favor of {@link #GRPC_SERVER_SENT_BYTES_PER_RPC}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the word "response" to be consistent with specs:
grpc.io/server/received_messages_per_rpc: Number of messages received in each RPC.
|
||
/** | ||
* Tag key that represents a client gRPC canonical status. Refer to | ||
* https://github.com/grpc/grpc/blob/master/doc/statuscodes.md. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question but I don't have a clear answer for it. I created census-instrumentation/opencensus-specs#95 to follow up.
*/ | ||
public static final MeasureLong GRPC_SERVER_STARTED_COUNT = | ||
Measure.MeasureLong.create( | ||
"grpc.io/server/started_count", "Number of server RPCs (streams) started", COUNT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
* @since 0.13 | ||
*/ | ||
public static void registerAllGrpcViews() { | ||
registerAllCumulativeViews(Stats.getViewManager()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Update RPC constants according to specs and our meeting today.
Equivalent changes to OC C++: census-instrumentation/opencensus-cpp#138.