-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: Update OpenCensus version. #4494
Merged
Merged
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
45be3ca
Update OpenCensus version.
songy23 5c3cdfb
Record stats for new RPC measure constants.
songy23 b1711d0
Fix tests related to RPC constants.
songy23 769b22b
Work around deprecation warning.
songy23 948f81a
Use an enum instead of boolean for a test helper method.
songy23 060dff8
Extract a few common tag values.
songy23 1903e0e
Update OpenCensus version for Bazel.
songy23 1b60c83
Remove one unnecessary SuppressWarnings(deprecation).
songy23 6275d31
Upgrade OpenCensus version to 0.14.0
songy23 2a2e16d
Record *_started_rpcs measurements.
songy23 b214f77
Put deprecated Census constants in a holder class.
songy23 bd052a4
Make the constructor of DeprecatedCensusConstants private.
songy23 cbde7a6
Update to latest oc version.
songy23 20f8de3
Temporarily remove the new constants.
songy23 2fea929
Suppress one more deprecation warning.
songy23 d1c9dff
Remove an unused enum.
songy23 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
80 changes: 80 additions & 0 deletions
80
core/src/main/java/io/grpc/internal/DeprecatedCensusConstants.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| /* | ||
| * Copyright 2018 The gRPC Authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package io.grpc.internal; | ||
|
|
||
| import com.google.common.annotations.VisibleForTesting; | ||
| import io.opencensus.contrib.grpc.metrics.RpcMeasureConstants; | ||
| import io.opencensus.stats.Measure.MeasureDouble; | ||
| import io.opencensus.stats.Measure.MeasureLong; | ||
| import io.opencensus.tags.TagKey; | ||
|
|
||
| /** Holder class for the deprecated OpenCensus constants. */ | ||
| @SuppressWarnings("deprecation") | ||
| @VisibleForTesting | ||
| public final class DeprecatedCensusConstants { | ||
|
|
||
| public static final TagKey RPC_STATUS = RpcMeasureConstants.RPC_STATUS; | ||
| public static final TagKey RPC_METHOD = RpcMeasureConstants.RPC_METHOD; | ||
|
|
||
| public static final MeasureLong RPC_CLIENT_ERROR_COUNT = | ||
| RpcMeasureConstants.RPC_CLIENT_ERROR_COUNT; | ||
| public static final MeasureDouble RPC_CLIENT_REQUEST_BYTES = | ||
| RpcMeasureConstants.RPC_CLIENT_REQUEST_BYTES; | ||
| public static final MeasureDouble RPC_CLIENT_RESPONSE_BYTES = | ||
| RpcMeasureConstants.RPC_CLIENT_RESPONSE_BYTES; | ||
| public static final MeasureDouble RPC_CLIENT_ROUNDTRIP_LATENCY = | ||
| RpcMeasureConstants.RPC_CLIENT_ROUNDTRIP_LATENCY; | ||
| public static final MeasureDouble RPC_CLIENT_SERVER_ELAPSED_TIME = | ||
| RpcMeasureConstants.RPC_CLIENT_SERVER_ELAPSED_TIME; | ||
| public static final MeasureDouble RPC_CLIENT_UNCOMPRESSED_REQUEST_BYTES = | ||
| RpcMeasureConstants.RPC_CLIENT_UNCOMPRESSED_REQUEST_BYTES; | ||
| public static final MeasureDouble RPC_CLIENT_UNCOMPRESSED_RESPONSE_BYTES = | ||
| RpcMeasureConstants.RPC_CLIENT_UNCOMPRESSED_RESPONSE_BYTES; | ||
| public static final MeasureLong RPC_CLIENT_STARTED_COUNT = | ||
| RpcMeasureConstants.RPC_CLIENT_STARTED_COUNT; | ||
| public static final MeasureLong RPC_CLIENT_FINISHED_COUNT = | ||
| RpcMeasureConstants.RPC_CLIENT_FINISHED_COUNT; | ||
| public static final MeasureLong RPC_CLIENT_REQUEST_COUNT = | ||
| RpcMeasureConstants.RPC_CLIENT_REQUEST_COUNT; | ||
| public static final MeasureLong RPC_CLIENT_RESPONSE_COUNT = | ||
| RpcMeasureConstants.RPC_CLIENT_RESPONSE_COUNT; | ||
|
|
||
| public static final MeasureLong RPC_SERVER_ERROR_COUNT = | ||
| RpcMeasureConstants.RPC_SERVER_ERROR_COUNT; | ||
| public static final MeasureDouble RPC_SERVER_REQUEST_BYTES = | ||
| RpcMeasureConstants.RPC_SERVER_REQUEST_BYTES; | ||
| public static final MeasureDouble RPC_SERVER_RESPONSE_BYTES = | ||
| RpcMeasureConstants.RPC_SERVER_RESPONSE_BYTES; | ||
| public static final MeasureDouble RPC_SERVER_SERVER_ELAPSED_TIME = | ||
| RpcMeasureConstants.RPC_SERVER_SERVER_ELAPSED_TIME; | ||
| public static final MeasureDouble RPC_SERVER_SERVER_LATENCY = | ||
| RpcMeasureConstants.RPC_SERVER_SERVER_LATENCY; | ||
| public static final MeasureDouble RPC_SERVER_UNCOMPRESSED_REQUEST_BYTES = | ||
| RpcMeasureConstants.RPC_SERVER_UNCOMPRESSED_REQUEST_BYTES; | ||
| public static final MeasureDouble RPC_SERVER_UNCOMPRESSED_RESPONSE_BYTES = | ||
| RpcMeasureConstants.RPC_SERVER_UNCOMPRESSED_RESPONSE_BYTES; | ||
| public static final MeasureLong RPC_SERVER_STARTED_COUNT = | ||
| RpcMeasureConstants.RPC_SERVER_STARTED_COUNT; | ||
| public static final MeasureLong RPC_SERVER_FINISHED_COUNT = | ||
| RpcMeasureConstants.RPC_SERVER_FINISHED_COUNT; | ||
| public static final MeasureLong RPC_SERVER_REQUEST_COUNT = | ||
| RpcMeasureConstants.RPC_SERVER_REQUEST_COUNT; | ||
| public static final MeasureLong RPC_SERVER_RESPONSE_COUNT = | ||
| RpcMeasureConstants.RPC_SERVER_RESPONSE_COUNT; | ||
|
|
||
| private DeprecatedCensusConstants() {} | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How is the migration handled on the consumer side in the Census implementation? For not to break the interoperability between gRPC and Census inside Google, it seems necessary for Census to read both the old and the new measurements, giving the new ones precedence. If that's the case, we could just remove the old measurements here.
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.
Unfortunately, inside Google the new measurements are not completely consistent with Census Java, and only the old measurements will be read at this moment. Even for open source, users could still export and read the old measurements so I think it's important to continue recording stats for the old ones.
There are some ongoing discussion about making the new measurements consistent with Census Java. /cc @dinooliva
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.
The artifact providing the gRPC views has separate methods for registering the old and new views, so users can choose when to upgrade them. It might be easier for users to upgrade if they could get data for both sets of views at the same time.
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 it possible to first change Census impls to convert the old measurements to the new ones, then switch gRPC to record the new measurements after all your customers have switched to the new impl and the new measurements? Then the migration of the measurements can go in line with the migration from NetworkEvent to MessageEvent (see my comment on that). Plus the gRPC code (esp. the tests) would be much cleaner if it ever has to deal with one API.
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'm not sure, but I thought that we wanted users to explicitly switch to the new views, since the change isn't transparent like the switch to MessageEvent. For example, the stats may be exported to a service that uses the view or measure names.
/cc @bogdandrutu
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.
Yes, like @sebright said, OpenCensus users could be exporting the old RPC constants to other backends and using them for alerting rules. Since the new and old RPC constants have different names, they will be different metrics on other monitoring systems (Stackdriver, Prometheus). We would like to allow users to explicitly choose old or new RPC constants and update their monitoring accordingly, as in our APIs (registerAllGrpcViews vs registerAllViews). So I think it's import to have stats for both of them.
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.
On top of that, as I mentioned in the previous comments, currently it's particularly difficult to map the old RPC constants to the new ones inside Google. For example, there are no *_count measures in the new constants.
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 see you don't have the "started count" measurements in the new constants, but it is a required metric inside Google.
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.
We removed started_count, finished_count and error_count in the new specs. There are some rationale behind why error_count and finished_count gets removed, but I cannot remember why we removed started_count.
@bogdandrutu @Ramonza should we consider putting back the started_count measure since it's required?
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 added *_started_rpcs constants to OpenCensus
v0.14.0, and upgraded the version here. Now the started_rpcs are recorded. PTAL at the two latest commits.