Adding opencensus grpc plugin with bazel support by Vizerai · Pull Request #15070 · grpc/grpc

Vizerai · 2018-04-14T02:24:44Z

This makes grpc depend on opencensus to build the plugin.

Make file support with git submodule could be added later.

This change is

grpc-testing · 2018-05-25T22:08:04Z

[microbenchmarks] No significant performance differences

markdroth · 2018-05-29T14:54:17Z

Reviewed 1 of 3 files at r18, 1 of 2 files at r19, 1 of 1 files at r20.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

include/grpcpp/opencensus.h, line 26 at r20 (raw file):

// Registers the OpenCensus plugin with gRPC, so that it will be used for future
// RPCs. This must be called before any views are created on the measures
// defined below. This will only be enabled if the binary includes the

The placement and wording of this comment makes it seem like it applies only to RegisterOpenCensusPlugin(), but it actually applies to everything in this file. Please update it to clarify that.

Also, "this will only be enabled" is a bit imprecise; let's make it clear that these symbols will not actually be present in the library until it was built with the specified target.

Comments from Reviewable

…port

grpc-testing · 2018-05-29T17:52:37Z

****************************************************************

libgrpc.so

     VM SIZE                                                  FILE SIZE
 ++++++++++++++ GROWING                                    ++++++++++++++
  [NEW]    +193 src/cpp/ext/filters/census/grpc_context.cc    +193  [NEW]
      [NEW]    +113 grpc_census_call_set_context                  +113  [NEW]
      [NEW]     +65 grpc_census_call_get_context                   +65  [NEW]
      [NEW]     +15 [Unmapped]                                     +15  [NEW]
  +0.0%     +16 [None]                                        +120  +0.0%

 -------------- SHRINKING                                  --------------
  [DEL]    -193 src/core/ext/census/grpc_context.cc           -193  [DEL]
      [DEL]    -113 grpc_census_call_set_context                  -113  [DEL]
      [DEL]     -65 grpc_census_call_get_context                   -65  [DEL]
      [DEL]     -15 [Unmapped]                                     -15  [DEL]

  +0.0%     +16 TOTAL                                         +120  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-05-29T17:59:41Z

[trickle] No significant performance differences

grpc-testing · 2018-05-29T18:43:56Z

[microbenchmarks] No significant performance differences

vjpai · 2018-06-04T17:01:36Z

bazel/grpc_deps.bzl

-            strip_prefix = "benchmark-5b7683f49e1e9223cf9927b24f6fd3d6bd82e3f8",
-            url = "https://github.com/google/benchmark/archive/5b7683f49e1e9223cf9927b24f6fd3d6bd82e3f8.tar.gz",
+            strip_prefix = "benchmark-9913418d323e64a0111ca0da81388260c2bbe1e9",
+            url = "https://github.com/google/benchmark/archive/9913418d323e64a0111ca0da81388260c2bbe1e9.tar.gz",


Any reason for this change?

There were incompatibilities with the older version of the library.

vjpai · 2018-06-04T17:02:05Z

bazel/grpc_deps.bzl

-            strip_prefix = "abseil-cpp-cc4bed2d74f7c8717e31f9579214ab52a9c9c610",
-            url = "https://github.com/abseil/abseil-cpp/archive/cc4bed2d74f7c8717e31f9579214ab52a9c9c610.tar.gz",
+            strip_prefix = "abseil-cpp-cd95e71df6eaf8f2a282b1da556c2cf1c9b09207",
+            url = "https://github.com/abseil/abseil-cpp/archive/cd95e71df6eaf8f2a282b1da556c2cf1c9b09207.tar.gz",


Likewise, any reason for this change?

There were incompatibilities with the older version of the library.

markdroth · 2018-06-12T20:53:39Z

Reviewed 15 of 15 files at r21.
Review status: all files reviewed, 4 unresolved discussions (waiting on @markdroth, @Vizerai, @vjpai, @a11r, @dgquintas, and @nicolasnoble)

include/grpcpp/opencensus.h, line 24 at r17 (raw file):

Previously, Vizerai (Jim King) wrote…

Added comment. @nicolasnoble can you give input about how we can ensure this does not get installed in binary distributions?

Still need to resolve this.

include/grpcpp/opencensus.h, line 26 at r20 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

The placement and wording of this comment makes it seem like it applies only to RegisterOpenCensusPlugin(), but it actually applies to everything in this file. Please update it to clarify that.

Also, "this will only be enabled" is a bit imprecise; let's make it clear that these symbols will not actually be present in the library until it was built with the specified target.

Still need to resolve this.

Comments from Reviewable

nicolasnoble · 2018-06-12T23:54:29Z

include/grpcpp/opencensus.h, line 24 at r17 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Still need to resolve this.

As long as the file aren't listed in the build.yaml, it won't be listed in the distribution systems, such as the Makefile's "install" target.

Comments from Reviewable

Vizerai · 2018-06-13T05:51:12Z

Review status: all files reviewed, 4 unresolved discussions (waiting on @markdroth, @Vizerai, @vjpai, @a11r, and @dgquintas)

include/grpcpp/opencensus.h, line 26 at r20 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Still need to resolve this.

Comment has been reworded.

Comments from Reviewable

grpc-testing · 2018-06-13T06:11:30Z

****************************************************************

libgrpc.so

     VM SIZE                                                  FILE SIZE
 ++++++++++++++ GROWING                                    ++++++++++++++
  [NEW]    +193 src/cpp/ext/filters/census/grpc_context.cc    +193  [NEW]
      [NEW]    +113 grpc_census_call_set_context                  +113  [NEW]
      [NEW]     +65 grpc_census_call_get_context                   +65  [NEW]
      [NEW]     +15 [Unmapped]                                     +15  [NEW]
  +0.0%     +16 [None]                                        +120  +0.0%

 -------------- SHRINKING                                  --------------
  [DEL]    -193 src/core/ext/census/grpc_context.cc           -193  [DEL]
      [DEL]    -113 grpc_census_call_set_context                  -113  [DEL]
      [DEL]     -65 grpc_census_call_get_context                   -65  [DEL]
      [DEL]     -15 [Unmapped]                                     -15  [DEL]

  +0.0%     +16 TOTAL                                         +120  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-13T06:19:56Z

[trickle] No significant performance differences

grpc-testing · 2018-06-13T07:03:02Z

[microbenchmarks] No significant performance differences

markdroth

LGTM. Thanks for doing this!

Vizerai · 2018-06-14T20:44:04Z

Review status: 47 of 48 files reviewed, 4 unresolved discussions (waiting on @markdroth, @vjpai, @a11r, and @dgquintas)

include/grpcpp/opencensus.h, line 24 at r17 (raw file):

Previously, nicolasnoble (Nicolas Noble) wrote…

As long as the file aren't listed in the build.yaml, it won't be listed in the distribution systems, such as the Makefile's "install" target.

Done.

Comments from Reviewable

nicolasnoble · 2018-06-18T22:52:25Z

This is okay, I think.

vjpai

LGTM

Vizerai requested review from markdroth and nicolasnoble April 14, 2018 02:24

Vizerai requested review from a11r, dgquintas and vjpai as code owners April 14, 2018 02:24

This comment has been minimized.

Sign in to view

Vizerai force-pushed the filter_port branch from 6a43b40 to 1e8fd78 Compare April 16, 2018 22:32

grpc deleted a comment from grpc-testing Apr 16, 2018

Merge branch 'master' of https://github.com/Vizerai/grpc into filter_…

e501a3d

…port

vjpai reviewed Jun 4, 2018

View reviewed changes

update

f9f0b54

markdroth approved these changes Jun 13, 2018

View reviewed changes

nicolasnoble approved these changes Jun 18, 2018

View reviewed changes

Vizerai merged commit 01cbab6 into grpc:master Jun 19, 2018

Vizerai mentioned this pull request Jun 19, 2018

Removing grpc filter. census-instrumentation/opencensus-cpp#163

Merged

vjpai reviewed Jun 19, 2018

View reviewed changes

markdroth mentioned this pull request Jun 19, 2018

Allocate retry payload fields with subchannel call instead of with each batch. #15715

Merged

ncteisen mentioned this pull request Jun 19, 2018

Fixing trailing metadata ops. #15809

Merged

jtattermusch mentioned this pull request Jun 20, 2018

grpc_opencensus_plugin_test broken on Remote bazel tests #15815

Closed

markdroth mentioned this pull request Jun 20, 2018

CallStackInit and full filter functionality microbenchmarks for filters #15630

Closed

AspirinSJL mentioned this pull request Jun 21, 2018

Add load reporter for server load reporting #15196

Merged

srini100 added release notes: no Indicates if PR should not be in release notes and removed release notes: no Indicates if PR should not be in release notes labels Jul 18, 2018

lock bot locked as resolved and limited conversation to collaborators Oct 17, 2018

Conversation

Vizerai commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

grpc-testing commented May 25, 2018

Uh oh!

markdroth commented May 29, 2018

Uh oh!

grpc-testing commented May 29, 2018

Uh oh!

grpc-testing commented May 29, 2018

Uh oh!

grpc-testing commented May 29, 2018

Uh oh!

vjpai Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

Vizerai Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

vjpai Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

Vizerai Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

markdroth commented Jun 12, 2018

Uh oh!

nicolasnoble commented Jun 12, 2018

Uh oh!

Vizerai commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

grpc-testing commented Jun 13, 2018

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Vizerai commented Jun 14, 2018

Uh oh!

nicolasnoble commented Jun 18, 2018

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Vizerai commented Apr 14, 2018 •

edited

Loading