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

Move GetSpanFromServerContext() to public header. #15984

Merged
merged 2 commits into from
Aug 21, 2018
Merged

Conversation

g-easy
Copy link
Contributor

@g-easy g-easy commented Jul 11, 2018

Move GetSpanFromServerContext() to public header.

@g-easy
Copy link
Contributor Author

g-easy commented Jul 11, 2018

Currently, consumers of this code need to do:

#include <grpcpp/opencensus.h>
#include "src/cpp/ext/filters/census/grpc_plugin.h"

The first line looks okay, but the second line is a bit odd (e.g. no grpcpp/ prefix). Is the second one considered a public header?

Copy link
Contributor

@Vizerai Vizerai left a comment

Choose a reason for hiding this comment

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

LGTM

@Vizerai
Copy link
Contributor

Vizerai commented Jul 12, 2018

Nobody said anything against having grpc_plugin.h within src/cpp when I had the PR reviewed, so I assumed that was ok.

@grpc-testing
Copy link

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@@ -19,6 +19,8 @@
#ifndef GRPCPP_OPENCENSUS_H
#define GRPCPP_OPENCENSUS_H

#include "opencensus/trace/span.h"

Copy link
Member

Choose a reason for hiding this comment

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

Because of issues including the Abseil dependence, let me recommend that there should be an explicit check that we're actually building with bazel. Something like:

#ifndef GRPC_BAZEL_BUILD
#error OpenCensus for gRPC is only supported when building with bazel
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@g-easy
Copy link
Contributor Author

g-easy commented Aug 10, 2018

No idea what this "mergeable" business is. I was expecting the followup commit to become a "fixup" in the git rebase sense.

@vjpai vjpai added release notes: no Indicates if PR should not be in release notes kokoro:run labels Aug 17, 2018
@grpc-testing
Copy link

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,952,998      Total (=)      1,952,998

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,664,799      Total (>)     10,664,797

 No significant differences in binary sizes


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@vjpai vjpai merged commit ccc6ee3 into grpc:master Aug 21, 2018
g-easy added a commit to g-easy/opencensus-cpp that referenced this pull request Aug 21, 2018
GetSpanFromServerContext() comes from <grpcpp/opencensus.h> as of
grpc/grpc#15984
g-easy added a commit to census-instrumentation/opencensus-cpp that referenced this pull request Aug 21, 2018
GetSpanFromServerContext() comes from <grpcpp/opencensus.h> as of
grpc/grpc#15984
@g-easy g-easy deleted the hdrs branch August 29, 2018 08:28
@lock lock bot locked as resolved and limited conversation to collaborators Nov 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants