Skip to content
This repository was archived by the owner on Dec 23, 2023. It is now read-only.

gRPC metrics: Add measures and views for real-time metrics in streaming RPCs.#1563

Merged
songy23 merged 1 commit intocensus-instrumentation:masterfrom
songy23:grpc-stream-metrics
Nov 20, 2018
Merged

gRPC metrics: Add measures and views for real-time metrics in streaming RPCs.#1563
songy23 merged 1 commit intocensus-instrumentation:masterfrom
songy23:grpc-stream-metrics

Conversation

@songy23
Copy link
Copy Markdown
Contributor

@songy23 songy23 commented Nov 7, 2018 •
edited
Loading

Blocked waiting on census-instrumentation/opencensus-specs#206.

The new views won't be included in the default view set and need to be registered separately.

@songy23 songy23 added this to the Release 0.18.0 milestone Nov 7, 2018
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 7, 2018 •
edited
Loading

Codecov Report

Merging #1563 into master will decrease coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1563      +/-   ##
============================================
- Coverage     82.48%   82.21%   -0.28%     
+ Complexity     1792     1733      -59     
============================================
  Files           262      259       -3     
  Lines          8215     8107     -108     
  Branches        793      770      -23     
============================================
- Hits           6776     6665     -111     
- Misses         1193     1209      +16     
+ Partials        246      233      -13
Impacted Files Coverage Δ Complexity Δ
...a/io/opencensus/contrib/grpc/metrics/RpcViews.java 71.42% <ø> (ø) 11 <0> (ø) ⬇️
...nsus/contrib/grpc/metrics/RpcMeasureConstants.java 100% <100%> (ø) 1 <0> (ø) ⬇️
...ncensus/contrib/grpc/metrics/RpcViewConstants.java 100% <100%> (ø) 1 <0> (ø) ⬇️
...java/io/opencensus/contrib/http/HttpExtractor.java 0% <0%> (-100%) 0% <0%> (-1%)
...census/trace/propagation/PropagationComponent.java 80% <0%> (-20%) 3% <0%> (ø)
...monitoredresource/util/MonitoredResourceUtils.java 14.28% <0%> (-16.97%) 1% <0%> (-1%)
...sus/implcore/trace/export/ExportComponentImpl.java 85% <0%> (-15%) 9% <0%> (-1%)
.../opencensus/impl/internal/DisruptorEventQueue.java 79.54% <0%> (-11.37%) 5% <0%> (-1%)
...rter/stats/stackdriver/StackdriverExportUtils.java 79.06% <0%> (-8.44%) 37% <0%> (-6%)
...src/main/java/io/opencensus/resource/Resource.java 82.75% <0%> (-8.36%) 14% <0%> (-8%)
... and 21 more

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 1e73f47...7fc858e. Read the comment docs.

@songy23 songy23 force-pushed the grpc-stream-metrics branch from 3a0e68a to d0bb674 Compare November 9, 2018 17:45
This was referenced Nov 9, 2018
Merged
Closed
@songy23 songy23 force-pushed the grpc-stream-metrics branch from be50c5d to bd37f10 Compare November 13, 2018 18:43
@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Nov 13, 2018 •
edited
Loading

Note that we must define the *_COUNT measures in Java, in order to do the mapping to internal resources. This may not be the case for other languages and the *_COUNT measures are not in the specs.

@songy23 songy23 added the kokoro:force-run Label to force a Kokoro build immediately. label Nov 13, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Label to force a Kokoro build immediately. label Nov 13, 2018
@songy23 songy23 mentioned this pull request Nov 14, 2018
Closed
@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Nov 14, 2018

Reverted bd37f10. We can do some internal mapping to get the counts.

@songy23 songy23 force-pushed the grpc-stream-metrics branch from bd37f10 to d14383e Compare November 14, 2018 01:42
zhangkun83
zhangkun83 reviewed Nov 14, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@zhangkun83 zhangkun83 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, although we need to resolve the question on the other PR as it may affect this one.

Comment thread contrib/grpc_metrics/src/main/java/io/opencensus/contrib/grpc/metrics/RpcMeasureConstants.java Outdated
public static final MeasureDouble GRPC_CLIENT_SENT_BYTES_PER_MESSAGE =
Measure.MeasureDouble.create(
"grpc.io/client/sent_bytes_per_message",
"Total bytes sent, recorded for each message in the RPC.",
Copy link
Copy Markdown
Contributor

@zhangkun83 zhangkun83 Nov 14, 2018

Choose a reason for hiding this comment

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

There is no "the RPC" to refer to. I would instead say "recorded for each RPC message".

Copy link
Copy Markdown
Contributor Author

@songy23 songy23 Nov 14, 2018

Choose a reason for hiding this comment

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

Fixed.

songy23
songy23 commented Nov 14, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Yes, I'll hold off this PR until we have an agreement on census-instrumentation/opencensus-specs#206 (comment). After that all the following steps should be trivial.

Comment thread contrib/grpc_metrics/src/main/java/io/opencensus/contrib/grpc/metrics/RpcMeasureConstants.java Outdated
public static final MeasureDouble GRPC_CLIENT_SENT_BYTES_PER_MESSAGE =
Measure.MeasureDouble.create(
"grpc.io/client/sent_bytes_per_message",
"Total bytes sent, recorded for each message in the RPC.",
Copy link
Copy Markdown
Contributor Author

@songy23 songy23 Nov 14, 2018

Choose a reason for hiding this comment

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

Fixed.

@songy23 songy23 changed the title gRPC metrics: Add per-message metrics. gRPC metrics: Add measures and views for real-time metrics in streaming RPCs. Nov 20, 2018
@songy23 songy23 force-pushed the grpc-stream-metrics branch from db02c99 to 43b0edc Compare November 20, 2018 00:49
@songy23 songy23 requested review from bogdandrutu and removed request for Vizerai November 20, 2018 00:52
@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Nov 20, 2018

Updated per census-instrumentation/opencensus-specs#212, PTAL.

@songy23 songy23 mentioned this pull request Nov 20, 2018
Merged
zhangkun83
zhangkun83 approved these changes Nov 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@zhangkun83 zhangkun83 left a comment

Choose a reason for hiding this comment

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

LGTM

@songy23 songy23 assigned songy23 and unassigned zhangkun83 Nov 20, 2018
@songy23 songy23 force-pushed the grpc-stream-metrics branch from 43b0edc to 7fc858e Compare November 20, 2018 02:28
@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Nov 20, 2018

Thanks!

@songy23 songy23 merged commit 8b1fd5b into census-instrumentation:master Nov 20, 2018
@songy23 songy23 deleted the grpc-stream-metrics branch November 20, 2018 04:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

1 more reviewer

@zhangkun83 zhangkun83 zhangkun83 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bogdandrutu bogdandrutu

@songy23 songy23

Labels

cla: yes gRPC

Projects

None yet

Milestone

Release 0.18.0

Development

Successfully merging this pull request may close these issues.

8 participants

@songy23 @codecov-io @zhangkun83 @flands @bogdandrutu @googlebot @Vizerai @kokoro-team