Skip to content
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

Added RPC Measure and View Constants #889

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

aasiyahf
Copy link

We (myself and @anamnavi) have been working on resolving issue #276 alongside Microsoft's Quantum Computing Team for code review, quality control, etc. in relation to RPC zpages written in Python.

As per the requirements of the opencensus specs document, we implemented the RPC View and Measure Constants. You can find them within opencensus/grpc under the names rpc_measure_constants.py and rpc_view_constants.py. We have included test files under the names rpc_m_c_test.py and rpc_v_c_test.py, respectively.

For further inquiry, you can reach me at aasiyahf@gmail.com or anfeisal@ncsu.edu and @anamnavi at anam.naviyou@gmail.com or anavied@ncsu.edu.

@aasiyahf aasiyahf requested review from c24t, reyang, songy23 and a team as code owners April 30, 2020 20:22
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@anamnavi
Copy link

@googlebot I consent

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@cdietschrun
Copy link

@songy23 @c24t @reyang Hi all, I'm the MSFT contact that was working with these students on a senior design project with this as one of the outputs (Hi @reyang , we met last summer). They've officially finished their projects, so I'm trying to help them push their changes through into the repo. It looks to me that they've cleaned up the code here, and it's unclear why the travis build is failing on a unit test.

Can you help let us know if we need to do anything for that particular build/test?

@cdietschrun
Copy link

cdietschrun commented May 14, 2020

2 week ping, would love to drive this one through if at all possible, if anything's needed on our side, since the authors have graduated by now.

@lzchen
Copy link
Contributor

lzchen commented Jun 29, 2020

@cdietschrun
Try rebasing and see if the error is still there.

@cdietschrun
Copy link

Looks clean now

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all of these be under opencensus/stats?

# limitations under the License.


# import rpc_measure_constants
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these commented out imports for?

Define constants used to define Measures below
see specs in documentation for opencensus-python
"""
byte = "by"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
byte = "by"
byte = "By"

# (always 1 for non-streaming RPCs)
self.grpc_client_sent_messages_per_rpc = MeasureInt(
name="grpc.io/client/sent_messages_per_rpc",
description="Number of messages sent in the RPC",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description="Number of messages sent in the RPC",
description="Number of messages sent in the RPC (always 1 for non-streaming RPCs)",

# per RPC (always 1 for non-streaming RPCs)
self.grpc_client_received_messages_per_rpc = MeasureInt(
name="grpc.io/client/received_messages_per_rpc",
description="Number of response messages received per RPC",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description="Number of response messages received per RPC",
description="Number of response messages received per RPC (always 1 for non-streaming RPCs)",

# have the same value as "grpc.io/server/latency"
self.grpc_client_server_latency = MeasureFloat(
name="grpc.io/client/server_latency",
description="Server latency in msecs",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description="Server latency in msecs",
description="Propagated from the server and should have the same value as "grpc.io/server/latency",

# including those that have not completed
self.grpc_client_started_rpcs = MeasureInt(
name="grpc.io/client/started_rpcs",
description="Number of started client RPCs.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the exact descriptions that are listed in the specs.

description="Received bytes per RPC",
columns=[rpc_m_c.grpc_server_method],
measure=rpc_m_c.grpc_server_received_bytes_per_rpc,
aggregation=sum)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Histogram?

description="Sent bytes per RPC",
columns=[rpc_m_c.grpc_server_method],
measure=rpc_m_c.grpc_server_sent_bytes_per_method,
aggregation=sum)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Histogram?

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some outstanding comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants